Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Sequential navigation in edit history #13418

Merged
merged 15 commits into from
Jun 28, 2017
Merged
3 changes: 3 additions & 0 deletions src/editor/Editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,9 @@ define(function (require, exports, module) {
this._codeMirror.on("cursorActivity", function (instance) {
self.trigger("cursorActivity", self);
});
this._codeMirror.on("beforeSelectionChange", function (instance, selectionObj) {
self.trigger("beforeSelectionChange", selectionObj);
});
this._codeMirror.on("scroll", function (instance) {
// If this editor is visible, close all dropdowns on scroll.
// (We don't want to do this if we're just scrolling in a non-visible editor
Expand Down
359 changes: 359 additions & 0 deletions src/extensions/default/NavigationAndHistory/NavigationProvider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,359 @@
/*
* Copyright (c) 2017 - present Adobe Systems Incorporated. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
* to deal in the Software without restriction, including without limitation
* the rights to use, copy, modify, merge, publish, distribute, sublicense,
* and/or sell copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.
*
*/

/**
* Manages Editor navigation history to aid back/fwd movement between the edit positions
* in the active project context. The navigation history is purely in-memory and not
* persisted to file system when a project is being closed.
*/
define(function (require, exports, module) {
"use strict";

var Strings = brackets.getModule("strings"),
MainViewManager = brackets.getModule("view/MainViewManager"),
DocumentManager = brackets.getModule("document/DocumentManager"),
DocumentCommandHandlers = brackets.getModule("document/DocumentCommandHandlers"),
EditorManager = brackets.getModule("editor/EditorManager"),
Editor = brackets.getModule("editor/Editor"),
ProjectManager = brackets.getModule("project/ProjectManager"),
CommandManager = brackets.getModule("command/CommandManager"),
Commands = brackets.getModule("command/Commands"),
Menus = brackets.getModule("command/Menus"),
KeyBindingManager = brackets.getModule("command/KeyBindingManager"),
FileSystem = brackets.getModule("filesystem/FileSystem");

var KeyboardPrefs = JSON.parse(require("text!keyboard.json"));

// Command constants for navigation history
var NAVIGATION_JUMP_BACK = "navigation.jump.back",
NAVIGATION_JUMP_FWD = "navigation.jump.fwd";

// The latency time to capture an explicit cursor movement as a navigation frame
var NAV_FRAME_CAPTURE_LATENCY = 2000;

/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing * (/**)

* Contains list of most recently known cursor positions.
* @private
* @type {Array.<Object>}
*/
var jumpToPosStack = [];
Copy link
Contributor

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing * (/**)

* Contains list of most recently traversed cursor positions using NAVIGATION_JUMP_BACK command.
* @private
* @type {Array.<Object>}
*/
var jumpedPosStack = [],
Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

captureTimer,
activePosNotSynched = false,
Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: activePosNotSynced

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

jumpInProgress,
command_JumpBack,
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

command_JumpFwd,
cmMarkers = {};

/**
* Function to enable/disable navigation command based on cursor positions availability.
* @private
*/
function _validateNavigationCmds() {
if (jumpToPosStack.length > 0) {
command_JumpBack.setEnabled(true);
Copy link
Collaborator

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);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} else {
command_JumpBack.setEnabled(false);
}

if (jumpedPosStack.length > 0) {
command_JumpFwd.setEnabled(true);
} else {
command_JumpFwd.setEnabled(false);
}
}

/**
* Function to check existence of a file entry
* @private
*/
function _checkIfExist(entry) {
Copy link
Collaborator

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

Copy link
Collaborator Author

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 () {
`

var deferred = new $.Deferred(),
fileEntry = FileSystem.getFileForPath(entry.file);

if (entry.inMem) {
var indxInWS = MainViewManager.findInWorkingSet(entry.paneId, entry.file);
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Remove entry if InMemoryFile is not found in Working set
if (indxInWS === -1) {
deferred.reject();
} else {
deferred.resolve();
}
} else {
fileEntry.exists(function (err, exists) {
if (!err && exists) {
deferred.resolve();
} else {
deferred.reject();
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: indentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

return deferred.promise();
}

/**
* Prototype to capture a navigation frame and it's various data/functional attributues
*/
function NavigationFrame(editor, selectionObj) {
this.cm = editor._codeMirror;
this.file = editor.document.file._path;
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

this.inMem = editor.document.file.constructor.name === "InMemoryFile" ? true : false;
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this.paneId = editor._paneId;
this.uId = (new Date()).getTime();
this.selections = [];
this.bookMarkIds = [];
this._createMarkers(selectionObj.ranges);
this._bindEditor(editor);
}

/**
* Binds the lifecycle event listner of the editor for which this frame is captured
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: listener

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 😄

*/
NavigationFrame.prototype._bindEditor = function (editor) {
var self = this;
editor.on("beforeDestroy", function () {
self._backupSelectionRanges();
self.cm = null;
self.bookMarkIds = null;
});
};

/**
* Function to create CM TextMarkers for the navigated positions/selections.
* This logic is required to ensure that the captured navaigation positions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: navigation

* stay valid and contextual even when the actual document text mutates.
* The mutations which are handled here :
* -> Addition/Deletion of lines before the captured position
* -> Addition/Updation of characters in the captured selection
*/
NavigationFrame.prototype._createMarkers = function (ranges) {
Copy link
Contributor

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)
}

Copy link
Collaborator Author

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.

var range, index, bookMark;
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this.bookMarkIds = [];
for (index in ranges) {
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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

bookMark = this.cm.setBookmark(range.anchor, range.head);
this.bookMarkIds.push(bookMark.id);
} else {
this.cm.markText(range.anchor, range.head, {className: (this.uId)});
}
}
};

/**
* Function to actually convert the CM markers to CM positions which can be used to
* set selections or cursor positions in Editor.
*/
NavigationFrame.prototype._backupSelectionRanges = function () {
if (!this.cm) {
return;
}

this.selections = [];
var marker, selection, index;
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

var self = this;
var markers = this.cm.getAllMarks().filter(function (entry) {
if (entry.className === self.uId || self.bookMarkIds.indexOf(entry.id) !== -1) {
return entry;
}
});
for (index in markers) {
marker = markers[index];
selection = marker.find();
if (marker.type === "bookmark") {
this.selections.push({start: selection, end: selection});
} else {
this.selections.push({start: selection.from, end: selection.to});
}
}
};

/**
* Function to actually navigate to the position(file,selections) captured in this frame
*/
NavigationFrame.prototype.goTo = function () {
var self = this;
this._backupSelectionRanges();
jumpInProgress = true;
CommandManager.execute(Commands.FILE_OPEN, {fullPath: this.file, paneId: this.paneId}).done(function () {
EditorManager.getCurrentFullEditor().setSelections(self.selections, true);
_validateNavigationCmds();
}).always(function () {
jumpInProgress = false;
});
};


/**
* Function to capture a non-zero set of selections as a navigation frame.
* The assumptions behind capturing a frame as a navigation frame are :
*
* -> If it's set by user explicitly (using mouse click or jump to definition)
* -> By clicking on search results
* -> Change of cursor by keyboard navigation keys or actual edits are not captured.
*
* @private
*/
function _recordJumpDef(event, selectionObj) {
if (jumpInProgress) {
return;
}
// Reset forward navigation stack if we are capturing a new event
jumpedPosStack = [];
if (captureTimer) {
window.clearTimeout(captureTimer);
captureTimer = null;
}
// Ensure cursor activity has not happened because of arrow keys or edit
if (selectionObj.origin !== "+move" && (window.event && window.event.type !== "input")) {
captureTimer = window.setTimeout(function () {
jumpToPosStack.push(new NavigationFrame(event.target, selectionObj));
_validateNavigationCmds();
activePosNotSynched = false;
}, NAV_FRAME_CAPTURE_LATENCY);
} else {
activePosNotSynched = true;
}
}

/**
* Command handler to navigate backward
*/
function _navigateBack() {
if (!jumpedPosStack.length) {
if (activePosNotSynched) {
jumpToPosStack.push(new NavigationFrame(EditorManager.getCurrentFullEditor(), {ranges: EditorManager.getCurrentFullEditor()._codeMirror.listSelections()}));
}
}
var navFrame = jumpToPosStack.pop();
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 () {
jumpedPosStack.push(navFrame);
navFrame.goTo();
}).fail(function () {
_validateNavigationCmds();
CommandManager.execute(NAVIGATION_JUMP_BACK);
});
}
}

/**
* Command handler to navigate forward
*/
function _navigateFwd() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer _navigateForward() here

var navFrame = jumpedPosStack.pop();
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 () {
jumpToPosStack.push(navFrame);
navFrame.goTo();
}).fail(function () {
_validateNavigationCmds();
CommandManager.execute(NAVIGATION_JUMP_FWD);
});
}
}

/**
* Function to initialize navigation menu items.
* @private
*/
function _initNavigationMenuItems() {
var menu = Menus.getMenu(Menus.AppMenuBar.NAVIGATE_MENU);
menu.addMenuItem(NAVIGATION_JUMP_BACK, "", Menus.AFTER, Commands.NAVIGATE_PREV_DOC);
menu.addMenuItem(NAVIGATION_JUMP_FWD, "", Menus.AFTER, NAVIGATION_JUMP_BACK);
}

/**
* Function to initialize navigation commands and it's keyboard shortcuts.
* @private
*/
function _initNavigationCommands() {
CommandManager.register(Strings.CMD_NAVIGATE_BACKWARD, NAVIGATION_JUMP_BACK, _navigateBack);
CommandManager.register(Strings.CMD_NAVIGATE_FORWARD, NAVIGATION_JUMP_FWD, _navigateFwd);
command_JumpBack = CommandManager.get(NAVIGATION_JUMP_BACK);
command_JumpFwd = CommandManager.get(NAVIGATION_JUMP_FWD);
command_JumpBack.setEnabled(false);
command_JumpFwd.setEnabled(false);
KeyBindingManager.addBinding(NAVIGATION_JUMP_BACK, KeyboardPrefs[NAVIGATION_JUMP_BACK]);
KeyBindingManager.addBinding(NAVIGATION_JUMP_FWD, KeyboardPrefs[NAVIGATION_JUMP_FWD]);
_initNavigationMenuItems();
}

/**
* Function to request a navigation frame creation explicitly.
* @private
*/
function _captureFrame(editor) {
// Capture the active position now if it was not captured earlier
if ((activePosNotSynched || !jumpToPosStack.length) && !jumpInProgress) {
jumpToPosStack.push(new NavigationFrame(editor, {ranges: editor._codeMirror.listSelections()}));
}
}

/**
* Handle Active Editor change to update navigation information
* @private
*/
function _handleActiveEditorChange(event, current, previous) {
if (previous && previous._paneId) { // Handle only full editors
previous.off("beforeSelectionChange", _recordJumpDef);
_captureFrame(previous);
_validateNavigationCmds();
}

if (current && current._paneId) { // Handle only full editors
activePosNotSynched = true;
current.on("beforeSelectionChange", _recordJumpDef);
}
}

function _handleProjectOpen() {
jumpToPosStack = [];
jumpedPosStack = [];
}

function _initHandlers() {
EditorManager.on("activeEditorChange", _handleActiveEditorChange);
ProjectManager.on("projectOpen", _handleProjectOpen);
}

function init() {
_initNavigationCommands();
_initHandlers();
}

exports.init = init;
});
10 changes: 10 additions & 0 deletions src/extensions/default/NavigationAndHistory/keyboard.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,15 @@
"key": "Cmd-Shift-[",
"platform": "mac"
}
],
"navigation.jump.back": [
{
"key": "Alt-I"
}
],
"navigation.jump.fwd": [
{
"key": "Alt-Shift-I"
}
]
}
Loading