Open Recent Files #12012

Merged
merged 29 commits into from May 23, 2016

Projects

None yet

9 participants

@swmitra
Contributor
swmitra commented Dec 17, 2015

This PR is aimed to provide the missing 'Open Recent Files' feature in brackets.
This feature is necessary when a user enables No Distractions mode. The MROF(Most recently opened files) list can be accessed and navigated in multiple ways ( using mouse only , using keyboard only and a mix of both).

Using mouse only

  • Select File->Open Recent to launch the MROF list ( refer to the screen shot below )
    filemenu
    and select(Click) on the file you want to open(refer to the list screen shot ).
    mroflist
  • The list can be closed using the 'X' button located at the top-right corner.
  • The list can be cleared using the 'Clear All' button located at the bottom-right corner.
  • The footer area shows the full path of the file selected/highlighted.

Mouse + Keyboard

  • press Alt+O to launch the MROF list.
  • Select the files using mouse left click or navigate using left/right arrow keys.

Keyboard only

  • Press Alt+Shift+N to navigate next in the file list.
  • Press Alt+Shift+P to navigate previous in the file list.
  • In this mode the MROF list pop over has auto dismissal after 1.5 secs when no navigation is performed.
  • Navigate to the file you want to open ( visual confirmation via MROF list highlight ) and the feature takes care of opening the editor and put the cursor in the last know position.
  • In this mode as the MROF list is for visual confirmation only , close and clear button is hidden in this mode ( refer to the screen shot below).
    mroflist k

NOTE
The file icons appearing in the screen shots is not part of brackets core. If any extension is installed which provides custom styling and icons to WorkingSetView as providers , MROF list borrows the same from WorkingSetView. (refer to the screen shot without icons below )
mroflist_base

@swmitra
Contributor
swmitra commented Dec 17, 2015
@petetnt
Member
petetnt commented Dec 17, 2015

Great stuff @swmitra, I've been missing for such a feature for a long time!

Things right off the top:

  • Highlights on long filenames don't show correctly
    image
  • When you select an item and click elsewhere on the dialog, the extension loses the highlight color but the filename doesn't (see the above screenshot)
  • Should this be an modal window (with a darkened backdrop). Currently it looks a bit weird because it feels like you could drag it around, but you cannot. Other choice would be a dropdown list, similar to the the quick open bar.
    • If it will be an modal, it should look like the other modals I guess
  • There are some z-index problems with other modal windows
    image
@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
...t/NavigationAndHistory/html/recentfiles-template.html
@@ -0,0 +1,165 @@
+<style>
@petetnt
petetnt Dec 17, 2015 Member

The styles should probably be in its own CSS file and then loaded with ExtensionUtils.loadStyleSheet(module, "styles/recent-files.css");

@swmitra
swmitra Dec 17, 2015 Contributor

I kept it purposefully in the template as it affects the DOM only when appended, to minimize any unintentional style override.

@petetnt
petetnt Dec 17, 2015 Member

As all the selectors are prepended with mrof (or start with #mrof-container ID) I don't think there should be anything that touches anything outside of the MROF-list 📝

@swmitra
Contributor
swmitra commented Dec 17, 2015

@petetnt Can you please check the last commit. Fixed all the UI corruption issues.

@petetnt
Member
petetnt commented Dec 17, 2015

@swmitra can confirm, 713a0dd fixed all the UI corruption issues (and the CSS thing I mentioned above) 👍

@swmitra
Contributor
swmitra commented Dec 17, 2015

@petetnt Thanks a lot for quick confirmation 😄

@sprintr
Contributor
sprintr commented Dec 17, 2015

👍 For this feature.

The dialog box used for this feature is different from the one used normally e.g (File -> Project Settings or View -> Themes). I believe it would be better to use a single style of dialog boxes.

The dialog boxes used for the Project Settings, Themes and extension manager are modal dialog boxes. I believe in this case we would need a modeless dialog box since it also opens the file in the background.

@sprintr sprintr and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
@@ -0,0 +1,402 @@
+/*
+ * Copyright (c) 2012 Adobe Systems Incorporated. All rights reserved.
@sprintr
sprintr Dec 17, 2015 Contributor

Update the copyright year to 2015 or maybe 2016 just to be on the safe side.

@abose
abose Dec 17, 2015 Contributor

@sprintr The theme used for this dialog is inline with the theme used in the project panel (side bar). That is why this dialog has a different look than the rest of the dialogs.

@sprintr
sprintr Dec 17, 2015 Contributor

Just don't feel comfortable with it maybe because it is the first time I see it. @larz0 or @GarthDB may give an opinion on this design change.

@sprintr sprintr and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ };
+ }
+
+ /**
+ * Determines if a file is dirty
+ * @private
+ * @param {!File} file - file to test
+ * @return {boolean} true if the file is dirty, false otherwise
+ */
+ function _isOpenAndDirty(file) {
+ // working set item might never have been opened; if so, then it's definitely not dirty
+ var docIfOpen = DocumentManager.getOpenDocumentForPath(file.fullPath);
+ return (docIfOpen && docIfOpen.isDirty);
+ }
+
+ /** Returns a 'context' object for getting/setting project-specific preferences */
@sprintr
sprintr Dec 17, 2015 Contributor

This comment should be multi-line.

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@sprintr sprintr and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ * Opens the next item in MROF list if pop over is visible else displays the pop over
+ * @private
+ */
+ function _moveNext() {
+ var $context, $next;
+ if ($mrofContainer) {
+ $context = $currentContext || $("#mrof-container > #mrof-list > li.highlight");
+ if ($context.length > 0) {
+ $next = $context.next();
+ if ($next.length > 0) {
+ $currentContext = $next;
+ _resetOpenFileTimer();
+ $next.find("a.mroitem").trigger("focus");
+ }
+ } else {
+ //WTF! (Worse than failure). We should not get here.
@sprintr
sprintr Dec 17, 2015 Contributor

😄

@swmitra
swmitra Dec 17, 2015 Contributor

No comments 😃

@sprintr
Contributor
sprintr commented Dec 17, 2015

When a file has unsaved changes the same behavior occurs that @petetnt pointed out earlier.

2015-12-17 21_08_20- c__users_aminullah_appdata_roaming_brackets_brackets json html - brackets

@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ if ($mrofContainer) {
+ $mrofContainer.remove();
+ $mrofContainer = null;
+ $currentContext = null;
+ if (EditorManager.getActiveEditor()) {
+ EditorManager.getActiveEditor().focus();
+ }
+ hideTimeoutVar = null;
+ }
+ }
+
+ /**
+ * Shows the current MROF list
+ * @private
+ */
+ function _createMROFDisplayList() {
@petetnt
petetnt Dec 17, 2015 Member

This function spans almost 100 lines. Most likely could use some refactoring ✏️

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ /**
+ * Shows the current MROF list
+ * @private
+ */
+ function _createMROFDisplayList() {
+ var $link, $newItem;
+ $mrofContainer = $(htmlTemplate).appendTo("#editor-holder");
+ var $mrofList = $mrofContainer.find("#mrof-list");
+
+ /**
+ * Focus handler for the link in list item
+ * @private
+ */
+ function _onFocus(event) {
+ $("#mrof-container > #mrof-list > li.highlight").removeClass("highlight");
+ $(event.target).parent().addClass("highlight");
@petetnt
petetnt Dec 17, 2015 Member

Save reused reference $(event.target).parent() to an variable here

function _onFocus(event) {
    var parent = $(event.target).parent();
    $("#mrof-container > #mrof-list > li.highlight").removeClass("highlight");
    parent.addClass("highlight");
    $mrofContainer.find("#recent-file-path").text(parent.data("path"));
    $currentContext = parent;
}
@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ $newItem.data("cursor", value.cursor);
+ $newItem.data("file", fileEntry);
+ $newItem.attr("title", value.file);
+
+ // Use the class providers(git e.t.c)
+ WorkingSetView.useClassProviders(data, $newItem);
+
+ if (_isOpenAndDirty(fileEntry)) {
+ $("<div class='file-status-icon dirty' style='position: static;float:left;margin-left: -5px;'></div>").prependTo($newItem);
+ }
+
+ $mrofList.append($newItem);
+ });
+
+ // Handlers for mouse events on the list items
+ $("#mrof-container > #mrof-list > li > a.mroitem").on("focus", _onFocus);
@petetnt
petetnt Dec 17, 2015 Member

Reused reference $("#mrof-container > #mrof-list > li > a.mroitem") can be again set to variable

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt commented on the diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ var htmlTemplate = require("text!html/recentfiles-template.html");
+
+ var $currentContext,
+ hideTimeoutVar,
+ openFileTimeoutVar;
+
+ // Delay in ms for hide timer when open recent files dialog shown from keyborad only commands
+ var HIDE_TIMEOUT_DELAY = 1500,
+ OPEN_FILE_DELAY = 600;
+
+ /*
+ * Contains list of most recently opened files and their last known cursor position
+ * @private
+ * @type {Array.<Object>}
+ */
+ var _mrofList = [],
@petetnt
petetnt Dec 17, 2015 Member

Nitpick: undeclared variables last (_mrofList, $mrofContainer, _activePaneId)

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ * @type {Array.<Object>}
+ */
+ var _mrofList = [],
+ $mrofContainer,
+ _activePaneId = null;
+
+ /**
+ * Opens a full editor for the given context
+ * @private
+ * @param {Object.<path, paneId, cursor>} contextData - wrapper to provide the information required to open a full editor
+ * @return {$.Promise} - from the commandmanager
+ */
+ function _openEditorForContext(contextData) {
+ return CommandManager.execute(Commands.FILE_OPEN, {fullPath: contextData.path,
+ paneId: contextData.paneId }).done(function () {
+ EditorManager.getActiveEditor().setCursorPos(contextData.cursor);
@petetnt
petetnt Dec 17, 2015 Member

Maybe save EditorManager.getActiveEditor() to an variable too.

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt and 1 other commented on an outdated diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ function _syncWithFileSystem() {
+ _mrofList = _mrofList.filter(function (e) {return e; });
+ Async.doSequentially(_mrofList, _checkExt, false);
+ _mrofList = _mrofList.filter(function (e) {return e; });
+ }
+
+ /**
+ * Hides the current MROF list if visible
+ * @private
+ */
+ function _hideMROFList() {
+ if ($mrofContainer) {
+ $mrofContainer.remove();
+ $mrofContainer = null;
+ $currentContext = null;
+ if (EditorManager.getActiveEditor()) {
@petetnt
petetnt Dec 17, 2015 Member

Maybe save EditorManager.getActiveEditor() to an variable too.

@swmitra
swmitra Dec 18, 2015 Contributor

Done.

@petetnt petetnt commented on the diff Dec 17, 2015
src/extensions/default/NavigationAndHistory/main.js
+ if (!err && exists) {
+ deferred.resolve();
+ } else {
+ _mrofList[index] = null;
+ deferred.reject();
+ }
+ });
+
+ return deferred.promise();
+ }
+
+ /**
+ * Checks whether entries in MROF list actually exists in fileSystem to prevent access to deleted files
+ * @private
+ */
+ function _syncWithFileSystem() {
@petetnt
petetnt Dec 17, 2015 Member

How does this function work? Looks weird 😸

@swmitra
swmitra Dec 18, 2015 Contributor

The filter call actually removes all null,undefined entries from the array. The Async call then ckecks for individual file entries which might have been deleted or removed from the actual file system and then marks those entries as null.
Then we filter the array again to remove null entries.

@petetnt
petetnt Dec 18, 2015 Member

@swmitra Neat, I was wondering about the first filter call but I understand now that it could contain null/undefined entries at that point too 👍

@swmitra
swmitra Dec 18, 2015 Contributor

Thank you @petetnt

@MarcelGerber
MarcelGerber May 16, 2016 Member

Maybe you should add some comments over here, as this may become a trap in the future.

@swmitra
Contributor
swmitra commented Dec 18, 2015

@sprintr Can you please check with aa5f73e commit. Fixed the UI corruption issue and most of the comments.
@petetnt Addressed comments.

@petetnt
Member
petetnt commented Dec 18, 2015

Found another issue while doing further testing against aa5f73e

  • The dialog can be opened multiple times (with ALT+O or File -> Open Recent Files) on top of itself. (After opening the dialog while the dialog is already open, one cannot remove the dialog without restarting Brackets (or removing it manually from the DOM), but that will be fixed automatically after preventing the wrong behavior).

The following is not an issue, more of an question regarding split view use:

  • File has been opened so that it ends up in the MROF-list. User closes the file from the working set. Later the user opens the file again from the MROF-list and it opens to the pane it was in previously:
    • Should files that have been removed from the working set be opened in the currently active pane instead of contextData.paneId?

Great job as always @swmitra 👍

@swmitra
Contributor
swmitra commented Dec 18, 2015

@petetnt Should be fixed now with 4653810 and 2nd point is also answered in the code. You are right, trusting the persisted pane info alone might land us in trouble.

@swmitra swmitra added this to the Release 1.6 milestone Dec 18, 2015
@swmitra swmitra self-assigned this Dec 18, 2015
@swmitra
Contributor
swmitra commented Dec 18, 2015

@larz0 Need your input on the UX

@abose abose modified the milestone: Release 1.7, Release 1.6 Feb 1, 2016
@abose
Contributor
abose commented Feb 1, 2016

Tagging @larz0 for UI review.

@larz0
Member
larz0 commented Feb 1, 2016

@swmitra @abose How do I get the open recent menu item to show up on Mac?

screen shot 2016-02-01 at 1 42 57 pm

@swmitra
Contributor
swmitra commented Feb 3, 2016

@larz0 Sorry for late reply. I had removed the keyboard workflow for some reason. Added it back now.
Short cut to open the panel is Alt+O. You can see a new menu item as well under File menu.

Alternatively there is a keyboard only workflow as well. Alt+N to navigate to next file in the list and Alt+P to navigate to previous file. Leaving the Alt key hides the panel in this workflow.

Requesting to please take ( pull) the code again as there are updates available ( menu item ). Also to navigate between some files to populate items in the list 😄

@larz0
Member
larz0 commented Feb 4, 2016

@swmitra ahh no problem, I'm the worst offender when it comes to replying late :)

UX Feedback:

  1. Change "Most Recently Opened Files" to "Recent Files" (Since the user just came from clicking "Open Recent")
  2. Use the multiplication sign (unicode: http://www.fileformat.info/info/unicode/char/00d7/index.htm) instead of "X" for the close button. It should match our panel's Close icon, just not the color. Make sure it's centered vertically properly.
  3. Maybe we don't need that many files, the recent file list is generally not this long; 20 is more than enough. Instead of putting the second most recent on the right, just fill the first column first then the second column. This is easier to scan in order instead of making eyes zig-zag.
  4. Directory Path could use a smaller font, try 11px (the same size as find-in-files results).
  5. Our dialogs have 4px rounded corners, so we should use the same rounded corners.

Hope this helps and thanks again, this is looking great 👍

(p.s. welcome to the team :D)

@swmitra
Contributor
swmitra commented Feb 8, 2016

Thanks a lot @larz0 👍
I will do the required changes and ping you for a quick confirmation again.

@nethip
Contributor
nethip commented May 10, 2016

@swmitra Did you get a chance to address the above comments? we should target this for 1.7

@swmitra
Contributor
swmitra commented May 11, 2016

@nethip I will update it today. Most of the changes suggested by @larz0 are addressed locally. I will push the changes today.

@nethip
Contributor
nethip commented May 11, 2016

thanks @swmitra

@ingorichter
Contributor

It looks good so far. I think that a couple of unit tests would be good for the new code.

@nethip
Contributor
nethip commented May 11, 2016

👍 for unit tests. And thanks @ingorichter for taking time in reviewing this PR.

@swmitra I have couple of comments. I will post the comments tomorrow, once I get to the office.

@nethip nethip and 1 other commented on an outdated diff May 12, 2016
src/editor/Editor.js
@@ -1155,13 +1155,15 @@ define(function (require, exports, module) {
* the "ch" parameter.
*/
Editor.prototype.setCursorPos = function (line, ch, center, expandTabs) {
+ var oldCursorPos = this.getCursorPos(true, "start");
@nethip
nethip May 12, 2016 Contributor

Looks like these the newly introduced events are not used in your PR? Do we still want to retain this change?

@swmitra
swmitra May 12, 2016 edited Contributor

@nethip This PR was actually taking care of move to next and previous context as well across files and within same files. I had de-scoped it to only recent files as the other one needs additional UI. But these changes are generic and will be usefull for every one who wants to capture cursor jump.

@nethip
nethip May 12, 2016 Contributor

@swmitra Sure! Move to another PR.

@nethip nethip and 2 others commented on an outdated diff May 12, 2016
src/extensions/default/NavigationAndHistory/main.js
@@ -0,0 +1,464 @@
+/*
+ * Copyright (c) 2015 Adobe Systems Incorporated. All rights reserved.
@nethip
nethip May 12, 2016 Contributor

Can we update this to 2016 😄

@swmitra
swmitra May 12, 2016 Contributor

Sure. I will make it evergreen 👍

@nethip
nethip May 12, 2016 Contributor

what is evergreen? 😄

@swmitra
swmitra May 12, 2016 Contributor

2016 - present 😄

@petetnt
petetnt May 12, 2016 Member

@nethip An evergreen format of 2016 - present has been used lately with the copyrights so one doesn't have to go and change them every year 📆

@nethip nethip commented on the diff May 12, 2016
src/extensions/default/NavigationAndHistory/main.js
+ $("#mrof-container > #mrof-list > li > a.mroitem:visited").last().trigger("focus");
+ }
+ //_resetHideTimer();
+ } else {
+ _createMROFDisplayList();
+ $mrofContainer.addClass("confirmation-mode");
+ //_startHideTimer();
+ $(window).on("keyup", _hideMROFListOnAltlUp);
+ }
+ }
+
+ /**
+ * Opens the previous item in MROF list if pop over is visible else displays the pop over
+ * @private
+ */
+ function _movePrev() {
@nethip
nethip May 12, 2016 Contributor

I guess _moveNext and _movePrev are almost the same except for .first() and .last(). Can we squash both the functions into a single parameterized function?

@swmitra
swmitra May 12, 2016 Contributor

@nethip Actually two differences. .next() ,.first() for next and .prev(), .last() for prev. If we merge and do it on the basis of a single parameter , we will further introduce branches in the if. That will increase the branch complexity further and the code will be unreadable. Infact, I had refactored this to two functions for readability.

@nethip
nethip May 12, 2016 Contributor

@swmitra I am with you on the readablity. From my experience, I can tell you that if a bug needs to be fixed, then the fix needs to be replicated at both the places.

I will leave this up to you, since this is a small function anyways.

@nethip nethip and 1 other commented on an outdated diff May 12, 2016
src/extensions/default/NavigationAndHistory/main.js
+ hideTimeoutVar,
+ openFileTimeoutVar,
+ activeEditor;
+
+ var _hideMROFList;
+
+ /**
+ * Opens a full editor for the given context
+ * @private
+ * @param {Object.<path, paneId, cursor>} contextData - wrapper to provide the information required to open a full editor
+ * @return {$.Promise} - from the commandmanager
+ */
+ function _openEditorForContext(contextData) {
+ // Open the file in the current active pane to prevent unwanted scenarios, fallback to the persisted paneId
+ // only when unable to determine active paneId (like empty working set at the launch)
+ var activePaneId = MainViewManager.getActivePaneId();
@nethip
nethip May 12, 2016 Contributor

I think this is a little misleading. Imagine I am in Pane 2 and I am switching the to file that is in Pane 1. This code will essentially move the file from Pane1 to Pane2 as you are relying on the active pane? Is this expected? I think the right thing to do is to figure out the existing opened document and switch to that document in the pane, the document is residing in.

@swmitra
swmitra May 12, 2016 edited Contributor

@nethip It will not move the file from Pane1 to Pane2. It will open it in Pane2 and retain the other editor in Pane 1 ( Same doc split 😄 ). We can't really determine the existing pane as with same doc split a single document can be opened in both the panes.

@nethip nethip and 1 other commented on an outdated diff May 12, 2016
src/extensions/default/NavigationAndHistory/main.js
+ _addToMROFList(previous);
+ }
+
+ if (current) {
+ _addToMROFList(current);
+ _activePaneId = MainViewManager.getActivePaneId();
+ }
+ });
+
+ ProjectManager.on("beforeProjectClose beforeAppClose", function () {
+ PreferencesManager.setViewState("openFiles", _mrofList, _getPrefsContext(), true);
+ _mrofList = [];
+ });
+
+ ProjectManager.on("projectOpen", function () {
+ _mrofList = PreferencesManager.getViewState("openFiles", _getPrefsContext()) || [];
@nethip
nethip May 12, 2016 Contributor

Is a new entry required in view state. Is it possible to rely on MainViewManager view states to determine the entires that need to go into our recent files window? That ways we don't have to worry about keeping the new view state in sync.

@swmitra
swmitra May 12, 2016 Contributor

@nethip A new entry is required in view state. MainViewManager keeps only the files which are in working set or currently in active edit context. That's the whole point of bringing in recent files notion as none of our data structures keeps closed file path. Moreover a closed file path in never captured in view state. So, if we restart brackets and don't keep the closed files path persisted, we can't recreate the history.

@nethip
nethip May 12, 2016 Contributor

I just realized I think you might want to preserve all files which are opened in the editor, but not in the working set. I think, it is OK to have a preference for this.

@nethip nethip and 1 other commented on an outdated diff May 12, 2016
src/extensions/default/NavigationAndHistory/main.js
+ var _hideMROFList;
+
+ /**
+ * Opens a full editor for the given context
+ * @private
+ * @param {Object.<path, paneId, cursor>} contextData - wrapper to provide the information required to open a full editor
+ * @return {$.Promise} - from the commandmanager
+ */
+ function _openEditorForContext(contextData) {
+ // Open the file in the current active pane to prevent unwanted scenarios, fallback to the persisted paneId
+ // only when unable to determine active paneId (like empty working set at the launch)
+ var activePaneId = MainViewManager.getActivePaneId();
+ return CommandManager.execute(Commands.FILE_OPEN, {fullPath: contextData.path,
+ paneId: activePaneId || contextData.paneId }).done(function () {
+ activeEditor = EditorManager.getActiveEditor();
+ activeEditor.setCursorPos(contextData.cursor);
@nethip
nethip May 12, 2016 Contributor

Do we have to explicitly set the focus? Is Commands.FILE_OPEN not care of that?

@swmitra
swmitra May 12, 2016 Contributor

Yes @nethip. Assume that this file was not in mru list. Then Brackets will put cursor on (1,1) , but as the context data was persisted and when the file was closed, we try to put back the cursor to the same place where it was exactly before the file was closed.

@nethip
Contributor
nethip commented May 12, 2016 edited

@swmitra I am done reviewing the code. Here are my comments after using your extension.

  • Dismiss the window once the user has selection the file in the recent files window
  • I don't see a reason why we don't have to hook Ctrl + Tab to this workflow. We might want to externalize the logic in our core, where extension can override Ctrl+Tab
  • I would love to have this working on the basis of Alt/Cmd + Tab OS task switching. So your window comes up on Alt/Cmd + Tab. With key held on Alt/Cmd key, the user can go forward/backward using tab or shift+tab key respectively. Upon releasing all the fingers from keys, the window should dismiss.
  • Make changes to UI to differentiate Panes, if split view is on.
  • I saw a bug, where upon doing a Ctrl+O, the list in the recent files window, is not up to date with the WorkingSetView. May be you might want to look at that.

Otherwise, this is exactly what I have been missing for a long time. Great Job! 👍

@swmitra
Contributor
swmitra commented May 12, 2016 edited

Thanks a lot @nethip for the detailed review 👍 and using the feature as well 👍

The last point you mentioned is indeed a bug as what we are doing now is, check the new view state ( which will be undefined for the first time) or initialize with empty array. I guess instead of empty we should start with the mru list.

Let me try to integrate the same shortcuts as you mentioned as that really sounds more intuitive 💯

@swmitra
Contributor
swmitra commented May 16, 2016

With the current commits , the shortcut and the workflow is changed a bit.

Mouse & Keyboard workflow

  • Alt + o or 'File->Open Recent' - to bring up the recent file list for mouse workflow.
  • Up/Down key to navigate or use pointer to select
  • On enter or selection, the entry is opened and pop over gets hidden
  • Popover can be dismissed by clicking the close 'x' link as well

KeyBoard Only workflow

  • Ctrl/Cmd + Tab to navigate next
  • Ctrl/Cmd + Shift + Tab to navigate previous
  • In this mode the popover hide is tied to Ctrl/Cmd key up or window blur

Additional features

  • Non-working set entries will have a dimmed look to separate them from working set entries.
  • If split view is on, recent file list will also show a split list but with MRU list navigation across.
  • Once split view is merged , even the recent files entries will be merged to first-pane
  • Clear button in the pop over , clears only the unlisted entries/ dimmed entries which are not part of working set.

NOTE
Existing command shortcuts for navigating in MRU list has been associated with this new feature as this is a super set of the current MRU list. But the current menu items for MRU list navigation has been retained with working set navigation.

Tagging @nethip @petetnt @ingorichter @abose @sprintr @zaggino @larz0 @MarcelGerber for more feedback. Please use this feature and let me know what you guys think.

@MarcelGerber
Member

Ah, haven't seen this before.
An immediate first thought: I really think the dialog should be dismissable via Esc.

@MarcelGerber MarcelGerber commented on an outdated diff May 16, 2016
src/base-config/keyboard.json
@@ -293,7 +296,7 @@
"platform": "mac"
}
],
- "navigate.nextDoc": [
+ "next.recent.files": [
@MarcelGerber
MarcelGerber May 16, 2016 Member

I do think recent.files.next (and the same for below) fits better with our mostly-hierarchical command naming.

@MarcelGerber
MarcelGerber May 16, 2016 Member

Also, normally extension keybindings are handled by the extension itself (see RecentProjects, 2 as an example)

@MarcelGerber MarcelGerber commented on an outdated diff May 16, 2016
...t/NavigationAndHistory/html/recentfiles-template.html
@@ -0,0 +1,15 @@
+<div id="mrof-container">
+ <div id="mrof-list-wrapper" class="quiet-scrollbars">
+ <div class="header">
+ <span>Recent Files</span>
+ <a class="close" id="mrof-list-close">×</a>
+ </div>
+ <span class="first pane-label"></span>
+ <span class="second pane-label"></span>
+ <ul id="mrof-list"></ul>
+ <div class="footer">
+ <span id="recent-file-path"></span>
+ <div class="btn-alt-quiet" id="clear-mrof-list" title="Clear recent files list">Clear</div>
@MarcelGerber
MarcelGerber May 16, 2016 Member

Make the strings "Recent Files", "Clear" and "Clear recent files list" translatable.

@MarcelGerber
Member

When pressing Up/Down arrows to select an item, and the list has enough items so it scrolls, the arrow keys scroll the list, which makes for a strange feeling (hope you can repro it, as it's kind of hard to explain)

@MarcelGerber MarcelGerber commented on the diff May 16, 2016
src/extensions/default/NavigationAndHistory/main.js
+ }
+
+ function _createFileEntries($mrofList) {
+ var data, fileEntry, $link, $newItem;
+ // Iterate over the MROF list and create the pop over UI items
+
+ // If we are in split view we might want to show the panes corresponding to the entries
+ var isPaneLabelReqd = MainViewManager.getPaneCount() > 1;
+
+ if (isPaneLabelReqd) {
+ $mrofContainer.addClass("split-mode");
+ $(".first.pane-label", $mrofContainer).text(MainViewManager.getPaneTitle("first-pane"));
+ $(".second.pane-label", $mrofContainer).text(MainViewManager.getPaneTitle("second-pane"));
+ }
+
+ $.each(_mrofList, function (index, value) {
@MarcelGerber
MarcelGerber May 16, 2016 Member

Nitpick: We usually use Lodash's _.forEach

@MarcelGerber MarcelGerber commented on an outdated diff May 16, 2016
src/extensions/default/NavigationAndHistory/main.js
+ }
+
+ //DocumentCommandHandlers.registerMRUListNavigator(MRUListNavigationProvider);
+
+ AppInit.appReady(function () {
+
+ ExtensionUtils.loadStyleSheet(module, "styles/recent-files.css");
+
+ // Command to show recent files list
+ CommandManager.register("Open Recent", SHOW_RECENT_FILES, _showRecentFileList);
+
+ // Keybooard only - Navigate to the next doc in MROF list
+ CommandManager.register("Next in Recent", NEXT_IN_RECENT_FILES, _moveNext);
+
+ // Keybooard only - Navigate to the prev doc in MROF list
+ CommandManager.register("Prev in Recent", PREV_IN_RECENT_FILES, _movePrev);
@MarcelGerber
MarcelGerber May 16, 2016 Member

These command names should be translatable as well.

@swmitra
Contributor
swmitra commented May 16, 2016

Thanks a lot @MarcelGerber for reviewing this feature 👍 .
I am addressing some of the comments specially the keyboard shortcut handling and making all strings translatable.
Also , I am adding escape key handling and will try to see the scrolling comment with arrow up/down.

@swmitra
Contributor
swmitra commented May 16, 2016

@MarcelGerber I have addressed most of the comments and fixed the scroll issue(had to navigate to a lot of files to simulate 😄 ) . Can you please have a look?
Thanks again for reviewing this feature 👍

@MarcelGerber
Member

Thank you! Looks good from my side now (I didn't have a thorough look at the code, though)

@ficristo
Member

I gave a try, a couple of questions:

  • why doesn't match the current modal styles (view/Themes, Extension Manager)? I like the thin header and footer but since I use a light theme seems odd at first
  • if I delete a file which is in the list and then I click on it, on the recent list, it says An error occured when trying to open the file <filename>. The file/directory could not be found. This 'error' is kind of expected. Maybe is fine as is, I don't know, but Notepad++ says that the file doesn't exists and ask to create it.
  • what is the behaviour of the clear button? To me it redorders in some ways the list; should remove all the files from the list?

By the way good job!

@nethip
Contributor
nethip commented May 17, 2016

@ficristo

if I delete a file which is in the list and then I click on it, on the recent list, it says An error occured when trying to open the file <filename>. The file/directory could not be found. This 'error' is kind of expected. Maybe is fine as is, I don't know, but Notepad++ says that the file doesn't exists and ask to create it.

Good catch 👍 and good feedback as well.

@swmitra
Contributor
swmitra commented May 18, 2016 edited

Thanks a lot @ficristo for trying this feature out. Really appreciated 👍

  • Regarding the deleted file point, is this the scenario where the file is deleted while the open file list is on(shown). As it has been designed to work with a snapshot of the list, we don't update the list in anyway while shown.

EDIT

Just saw the problem and it's a timing issue as the file system sync call is async, display list preparation should happen only after we are done with sync. Fixed and updated the PR.

Basically existence check is done on all entries before showing the list, but once shown we don't really check for existence.

  • The clear button clears all the un-linked entries (which are not part of working set). The recent file list is a super set of working set files so we don't really want to clear all the entries as that will restrict Ctrl+Tab navigation.
@ficristo
Member

Trying to clarifing my issue: I only think the message is a bit misleading.
It says an error is occured which is kind of expected and the message is about folder / files.

I think I'll be a bit confused about the clear button until I'll get accustomed 😛.

I don't have strong opinions about what to do (I didn't check the last commit...)

@swmitra
Contributor
swmitra commented May 18, 2016 edited

@ficristo The entry for non-existing files will not be shown in the list so that we can avoid clicking on a link pointing to an unreachable file entry(as per last commit).

Thanks a ton for using the feature and providing us good feedback 👍

@swmitra swmitra closed this May 18, 2016
@swmitra swmitra reopened this May 18, 2016
@swmitra
Contributor
swmitra commented May 18, 2016

// Sorry closed it by accident while pressing 'comment'.

@swmitra
Contributor
swmitra commented May 18, 2016

Added "recent.files.navigation" preference to enable/disable (true/false) recent files navigation.

@nethip
Contributor
nethip commented May 19, 2016

@swmitra Does it make sense it have something like navigation.enableNavigationDialog?

@nethip
Contributor
nethip commented May 19, 2016

@swmitra One last change. I think when changing preference for this, you need to rebuild the MRU by removing the non workspace items and by syncing your MRU with the workspace one.

@swmitra
Contributor
swmitra commented May 19, 2016

Thanks a lot @nethip for actively using the feature for review 👍
It really helps to provide a good feature experience in that way 😄

@swmitra
Contributor
swmitra commented May 19, 2016

Ping @nethip for a final check (hopefully 😄 )

@nethip
Contributor
nethip commented May 23, 2016

@swmitra I did give it a spin. LGTM.

Thanks @swmitra for this wonderful and a very helpful feature. 👍

@nethip nethip merged commit 59b33be into master May 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nethip nethip deleted the swmitra/NavigationHistory branch May 23, 2016
@abose
Contributor
abose commented May 24, 2016

Awesome!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment