-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Sequential navigation in edit history #13418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial code review, good stuff @swmitra!
captureTimer, | ||
activePosNotSynched = false, | ||
jumpInProgress, | ||
command_JumpBack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The camelcase
+ underscore
hybrid is a bit weird, would prefer just commandJumpBack
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
var jumpedPosStack = [], | ||
captureTimer, | ||
activePosNotSynched = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: values with defaults first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: activePosNotSynced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
function _validateNavigationCmds() { | ||
if (jumpToPosStack.length > 0) { | ||
command_JumpBack.setEnabled(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just do
commandJumpBack.setEnabled(jumpToPosStack.length > 0);
commandJumpFwd.setEnabled(jumpedPosStack.length > 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
fileEntry = FileSystem.getFileForPath(entry.file); | ||
|
||
if (entry.inMem) { | ||
var indxInWS = MainViewManager.findInWorkingSet(entry.paneId, entry.file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: prefer index
over indx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* Function to check existence of a file entry | ||
* @private | ||
*/ | ||
function _checkIfExist(entry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should null check for entry here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already checked at the call location -
if (navFrame) {
// We will check for the file existence now, if it doesn't exist we will jump back again
// but discard the popped frame as invalid.
_checkIfExist(navFrame).done(function () {
`
function NavigationFrame(editor, selectionObj) { | ||
this.cm = editor._codeMirror; | ||
this.file = editor.document.file._path; | ||
this.inMem = editor.document.file.constructor.name === "InMemoryFile" ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.inMem = editor.document.file.constructor.name === "InMemoryFile"
is sufficent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
/** | ||
* Binds the lifecycle event listner of the editor for which this frame is captured |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: listener
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 😄
* -> Addition/Updation of characters in the captured selection | ||
*/ | ||
NavigationFrame.prototype._createMarkers = function (ranges) { | ||
var range, index, bookMark; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: new variables on new lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
range = ranges[index]; | ||
// 'markText' has to used for a non-zero length position, if current selection is | ||
// of zero length use bookmark instead. | ||
if (range.anchor.line === range.head.line && range.anchor.ch === range.head.ch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: can anchor
or head
be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a cursor or selection, it can never be null and if there are no selections/cursor then the ranges would be an empty array.
} | ||
|
||
this.selections = []; | ||
var marker, selection, index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: new variables on new lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@petetnt Thanks a lot for reviewing this PR. It was really quick 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more nits, otherwise codewise starting to LGTM. Haven't had the time to test it yet.
// The latency time to capture an explicit cursor movement as a navigation frame | ||
var NAV_FRAME_CAPTURE_LATENCY = 2000; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing * (/**)
*/ | ||
var jumpToPosStack = []; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing * (/**)
} else { | ||
deferred.reject(); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: indentation: Tabs -> Spaces
if (!this.cm) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: L173-L177 indendation Tabs -> Spaces
} | ||
}); | ||
|
||
// Iterate over CM textmarkers and collate the updated(if?) positions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Indentation: tabs -> spaces
this.selections = []; | ||
var self = this; | ||
|
||
// Collate only the markers we used to mark selections/cursors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Indentation: tabs -> spaces
selection, | ||
index; | ||
|
||
// Reset selections first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Indentation: tabs -> spaces
I will not review, but I have a question. |
…ached. Fix all bugs found in unit testing.
@ficristo You are right 👍 . |
@petetnt Can you please have a look at the PR again and if possible use it. |
@nethip Can you please have a look at this PR? |
* @private | ||
* @type {Array.<Object>} | ||
*/ | ||
var jumpedPosStack = [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to initialize all the variables. here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good to me codewise, one nit and one typo. Also there's now mixed tabs and spaces in new code. Haven't had the time to test the functionality yet but I'll try soon.
/** | ||
* Command handler to navigate forward | ||
*/ | ||
function _navigateFwd() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer _navigateForward()
here
|
||
/** | ||
* Function to create CM TextMarkers for the navigated positions/selections. | ||
* This logic is required to ensure that the captured navaigation positions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: navigation
@petetnt Done. Fixed the last 2 nits and mixed tabs & spaces. |
@nethip Can you please have a look at this PR again? |
@swmitra I think the code part is fine. But here are few questions I had
Edited:
|
@nethip Thanks a ton for reviewing the PR 👍 I will try to answer the questions, please let me know if you think anything in the assumptions can go wrong.
Please let me know if you think we can go wrong in any of these or other situations. Thanks again for raising this questions, I could verify lot of the scenarios after looking at this 😄 |
@nethip I have added additional handling for external changes. Also, deciding on which pane the document should open on navigation is determined based on the current master editors association with pane. Please have a look at the additional changes. |
function _navigateForward() { | ||
var navFrame = jumpedPosStack.pop(); | ||
|
||
// Check if the poped frame is the current active frame or doesn't have any valid marker information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have an early return
here for the case when navFrame is null (!navFrame
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
function _hasNavBackFrames() { | ||
return (jumpedPosStack.length > 0 && jumpToPosStack.length > 0) | ||
|| (!jumpedPosStack.length && jumpToPosStack.length > 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get this code.
So, when you can't jump forward, there need to be at least two elements in jumpToPosStack
? Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't really get to actually testing it yet.
Just one note, and I hope you are aware of it: CodeMirror already has a kind of cursor/selection history using Ctrl-U / Ctrl-Shift-U
(Windows), so maybe we can utilize that a little? Maybe they already take care of all the markers internally?
* -> Addition/Deletion of lines before the captured position | ||
* -> Addition/Updation of characters in the captured selection | ||
*/ | ||
NavigationFrame.prototype._createMarkers = function (ranges) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createMarkers and reinstateMarkers share a lot of code. Maybe we can consolidate them a little more?
As I see it, this should work:
NavigationFrame.prototype._reinstateMarkers = function (editor) {
this.cm = editor._codeMirror;
this.paneId = editor._paneId;
this._createMarkers(this.selections)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was with range start and end placeholder names but handled it now to make use of same function.
} | ||
|
||
// Ensure cursor activity has not happened because of arrow keys or edit | ||
if (selectionObj.origin !== "+move" && (!window.event || (window.event && window.event.type !== "input"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify the logic here: !window.event || window.event.type !== "input"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored this code earlier but missed this condition completely. Changed it now.
// Check if we have reached MAX_NAV_FRAMES_COUNT | ||
// If yes, control overflow | ||
if (jumpToPosStack.length === MAX_NAV_FRAMES_COUNT) { | ||
var navFrame = jumpToPosStack.splice(0, 1)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use .shift()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
if (!jumpedPosStack.length) { | ||
if (activePosNotSynced) { | ||
currentEditPos = new NavigationFrame(EditorManager.getCurrentFullEditor(), {ranges: EditorManager.getCurrentFullEditor()._codeMirror.listSelections()}); | ||
jumpedPosStack.push(currentEditPos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Make the anonymous function inside setTimeout
a "real" function and use it here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use the anonymous function to create closure in the call back scope.
* @private | ||
* @type {Array.<Object>} | ||
*/ | ||
var jumpToPosStack = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make naming clearer, you might also use jumpStackForward
and jumpStackBackward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
*/ | ||
function NavigationFrame(editor, selectionObj) { | ||
this.cm = editor._codeMirror; | ||
this.file = editor.document.file._path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might call this filepath
or just path
to make it clear that this is not a file handle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. "filePath" would be more logical.
@marcelgerber Thanks a lot for reviewing this PR 👍 I have addressed most of the review comments. Please have a look at this PR and use it if possible. |
* add a tab with default extensions to extension manager * implement getDefaultExtensionPath function * enable and disable default extensions through a preference * use the preference when attempting to load an extension * do not allow disabling/enabling of themes * better way to handle themes * Upgraded version to 1.10 * Now search history is stored in preferences and user can go through the search history using key up and key down in search bar * ALF Automation (#13179) * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Fixed a minor issue * Addresses issue where style and script tags (and their contents) did not appear as collapsible in html mixed mode. * Add no-unsafe-negation rule and upgrade grunt-eslint to 19.0.0 * Added a check of Untitled doc in _getNormalizedFilename and _getDenormalizedFilename * ALF Automation (#13204) * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Replaced .border-radius with border-radius css in brackets_patterns_override.less This fixes #13227 * Addressed review comments and also added tests * Add jsx language ID to supported languages for JavaScript code hints * Style code (cleanup) * Add media queries to display search and filter below tabs * Change padding to be consistent with other elements' padding * Reset margin of filter dropdown and add top and bottom padding to div container * Prevent media queries from overlapping * Wrap search elements in a container & add responsiveness using flex * Use flex-item mixin * Move custom search bar width to media query * Add GitHub Brackets OAuth token to romove CLA pull request limit * Adding rest filter expression * Updated webplatform links in css.json from https://docs.webplatform.org/wiki/ to http://www.webplatform.org/docs/ (#13266) * Addresses #13264 where setting `saveFoldStates` preference to false caused the code folding extension to stop working. * CSS code hints in 'style' attribute value context * Append auth token only when available in travis environment (#13272) * Append auth token only when available in travis environment * Remove unused var * #7276 Live Preview highlight customization (#12949) * Live Preview visual customization (colors of highlight can be customized via user preferences) * PR improvements Remove trailing spaces, use Object.assign instead of manually iterating over remoteHighlight properties and remove quotes from object properties in default configuration of remoteHighlight * Allow to animate transform property via transition * Remove old vendor prefixes from transitionValues in RemoteFunctions.js * updateConfig RemoteFunctions.js method Enable injecting new config into remoteFunction when user changed preferences, without the need to restart the live preview session. * Show margin and paddings in live preview. * Do not show element if it has width or height equal to 0. A little bit of clean up * PR improvements calculateSize is renamed to sum. (better naming conventions are welcome). sum now takes an array as argument. It returns sum of parsed array elements, suffixed with „px”. * Redraw via requestAnimationFrame if element has transition / animation on it. * PR improvements. Set experimental to false by default, if no config specified (caused the test to crash). Minor code quality improvements, removed duplicated variables. * Fix ESLINT errors. * drawMarginRect and drawPaddingRect functions now calculate the require size of highlight elements instead of manaul calculations on each element. The code is shorter and more readable. Also, it’ll be easier to move drawing functions into separate file if necessary. * Temporary workaround for transform issue. * Formatting improvements * Fix font family name preference (#13279) * Update grunt-contrib-watch to 1.0.0 and clean a bit the task (#13215) * Scrapped QuickDocs data for css and html from MDN to css.json and html.json (#13268) * Scrapped summary and values of css properties from MDN to css.json * Migrated mozilla's extension of MDNDocs to Brackets with some minor changes * Removed redundant file * Fixed faling tests, modified a test to test HTML docs * Renamed all WebPlatformDocs strings to MDNDocs * Addressed review comments * Removed the usage of array for supported languages * Pre-release build for 1.10 (#13283) * Fix #13274 - make Brackets margin/padding highlight work like Chrome devtools one * Migrated missing Quick Docs Tests from PR #10036 (#13285) * Add inputStyle preference (#13216) * Add indentBlockComment preference. Support indent block comments on line comment command * Whitespace removal * AtRules, Pseudo elements and Pseudo selector code hints * Revert "Fix #13274 - make Brackets margin/padding remoteHighlight work like Chrome one" * Fix highlighting elements with border and transform-origin property. * Adress review comments and restructure using different extensions * Added Search History UI similar to Quick Open * Minor Change * Fixed some issues * Check for useTabChar and correct a comment. Add some tests * Handle multiline attribute value and add UNIT test for element styles * Search bar is visible after pressing arrow up and down keys * Added JSDocs and updated tests * Zero results are not highlighted red now * Added dropdown icon to toggle search History * fix the extension-manager-min-width variable * Handle SCSS mode seperately * Use a single preference * Add test for block comments * Correct a test * Check also if it is a line comment command * Addresses #13282 which caused performance issues when navigating between large folded files. A check is now performed to ensure we do not attempt to restore line folds for editors whose folds have already been initialised. * removed unused variable * Removed filtering in search dropdown * Return 0 from _firstNotWs * Changed url to access Japanese translated page (#13321) * Update Codemirror to version 5.25.2 * Addressed review comments * Removed Lint error * add: flow-root as value for display (#13334) * Changed Global.js to enable Native Menus for Linux * Adding unit tests for the newly added extensions * Fix for eslint errors * Extract pseudo context validation as a function * Enabled tests for Native Menus in Linux * Editor command handlers test (#13337) * EditorCommandHandlers-test: use testToggleLine everywhere * EditorCommandHandlers-test: use testToggleBlock everywhere * Now initial query is also added to searchHistory queue * Update CSS Code Hints Properties Add properties – `all` – `caret-color` – `hanging-punctuation` – `scroll-behavior` – `tab-size` – `user-select`. Add values: – `clone` and `slice` for `box-decoration-break` – `contents` for `display` Remove deprecated `padding-box` from `box-sizing`. * Update CSS Code Hints pseudo-selectors Add pseudo-classes – `:default` – `:dir()` – `:focus-within` – `:indeterminate` – `:matches()` – `:placeholder-shown`. Add pseudo-element `::placeholder`. Fix alphabetical order, space around `:`. * Update more Add – `grid-template` – `isolation` – `mix-blend-mode` Add `subgrid` for `display` Reorder `background-blend-mode` * Added test * Fix :matches(), add :fullscreen * Added check for length of dependencies also in npm-installer.js * Translated English to Japanese Comments (#13327) * Change parseInt to parseFloat everywhere where calculating box-model visualization to prevent glitches (#13353) * Fixed Project Manager failing tests * Brackets 1.10 Pre-release 2 * Fix box model regression (transform) (#13357) * Various additions and changes to Swedish interface strings (#13404) * Mixed changes and additions to Swedish interface strings * One addition and one fix * More additions and changes * Correct one word in sv/strings.js * Externalize Live preview hightlight settings string * ALF Automation (#13293) * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Fix root strings (#13435) * ALF Automation (#13434) * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Change in UUID fetching for first launches (#13419) * Fixing Mac compilation errors * Removing console message * Fixing linter errors in this file. * Add option for first highlight index in Quick Open and Search History (#13444) * Fixed #13437 * minor change * Passed first highlight index as paramater to QuickSearchField * Added jsdocs * Changed WebPlatform License to MDN License and respective strings * Use the correct border color in multifile Replace in Files bar with dark theme * Updated by ALF automation. (#13454) * In writeJSON (grunt), use CRLF on Windows (#13458) * Fixes translation error in Simplified Chinese Fixes /issues/13416 * Enable no-redeclare to eslint (#13452) * Added strings for translation for 1.10 Release * minor change * Add navigation strings. * Updated some strings * ALF Automation (#13471) * Updated by ALF automation. * Updated by ALF automation. * Added some more translations * Saurabh95/encoding support (#13412) * Now encoding is passed as parameter on file read and it is used by writefile in order to preserve encoding * UI wiring * Code Cleanup * Fixed failing tests * Added warning Dialog while changing encoding * Added some Linux specific changes * Fixed some linting errors * Reverted last commit * Minor changes * Now selected encoding is stored in state * Fixed lint error * Added some more encodings * Added some more encodings * Fixed lint errors * Removed duplicate encodings * Fixed failing tests * Used externalized strings * Addressed review comments * Added supported encodings file * Addressed review comments * Addressed review comments * More Robust UUID Handling (#13476) * More efficient UUID Handling: We need to backing up existing uuid so that we can accurately get the new user count, as the uuid is changed now and will effect the existing users as well. Also added robust checking for shell API availability before calling the shell API and if not present handling olderUUID in a different way. * Addressed review comments. * Now BOM is preserved for UTF-8 files (#13477) * Now BOM is preserved for UTF-8 files * Added error strings for failure in encode/decode and utf-16 * Removed utf-16 from encodings list * Addressed review comments * fix JP translation in comment tags (#13363) * New File and New Folder execute in project root if there's no selection (#12752) * New File and New Folder execute in project root if there's no selection * Add ProjectManager.getSelectedFileTreeItem * Add function to get file tree context instead * Polish translation update for brackets 1.10 (#13398) * Polish translation update for brackets 1.10 * Corrected some strings in pl translation * Added basic translation for core preferences section * Transform some polish strings in core preferences section * German translation (#13436) * zh-cn (#13473) * Update urls.js * Create Getting Started * Delete Getting Started * Create index.html * Create main.css * Add files via upload quick-edit.png * Problems panel: add an icon per problem to tell which type it is (#13430) * Sequential navigation in edit history (#13418) * First commit - sequential navigation in edit history * Fix lint errors * Handle undefined history entries * Added comments and some additional edge case handling * Update code review comments * Put a max navigation frame capture limit and control overflow once reached. Fix all bugs found in unit testing. * Fix indentation issues * Change spaces to tabs for indentation * Fix mixed tabs and spaces * Convert spaces to tab * resolve mixed tabs and spaces * Initialize variables * Handle external file changes and discard stale frames * Additional checks to validate the navigation frames * Address review comments * pre release build 3 * Fixed JS lint error (#13489) * Fix NavigationProvider throwing errors when doc.file is missing. Fixes #13491 (#13492) Signed-off-by: petetnt <[email protected]> * ALF Automation (#13474) * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Updated by ALF automation. * Adding extra check to handle null mrof list entries (#13495) * After changing encoding of a dirty file if user cancels the popup then encoding of file should not be changed (#13497) * After changing encoding of a dirty file if user cancels the popup then encoding of file should not be changed * Removed popup for dirty file encoding change * Now doc is reloaded if the path of the file being saved is same
Just downloaded the revision, but alt+i shows me this character ˆ?? OSX 10.11.6 |
Hi @schroef, the OSX shortcuts were improved / fixed in #13527, which didn't land to 1.10. You can change your shortcuts to match the ones in 13527 from Debug -> Show User Key Map and adding the following:
|
Seems shortcuts work not normally like in other applications. I need to pres cmd+alt+ left or right in a sequence in order to let it do something. I seem to be able to jump through documents. It doesnt always jump back to the complete history of it locations though |
@schroef Brackets have a slightly different implementation for navigation. We don't remember all the positions rather only explicit positions. For example when you do mouse click at a position and pause for a brief moment to read something, do a jump to def or switch file to work on some other context. |
We are going to change the OSX shorcuts and provide a small UI place holder as well in the next pre release ( within a week's time!) |
Well it doesnt store all the position i think. I've tried it couple times and sometimes is seems to work and sometimes not. I definitely dont see that history of 30 states. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a
This feature is something which I missed a lot! Remembering most recently used (interesting?) cursor positions and navigating there manually is really cumbersome.
This module adds the feature of remembering cursors/selections across full editors inside a project root and aids the user by providing 'Navigate Back'(Alt+I) and 'Navigate Fwd'(Alt+Shift+I) using the captured navigation frames.
Ping @petetnt @ficristo for review.