Use Ctrl+PageUp/PageDown to traverse files in list order #11223

Merged
merged 7 commits into from Jun 30, 2015

Projects

None yet

6 participants

@MarcelGerber
Member

For #6273.

This adds a shortcut (Ctrl/Cmd + PageUp/PageDown) to traverse your open files (in all panes) in list order. The shortcut itself is pretty much universal and I found it working like this in Google Chrome, Sublime Text 3 and Notepad++ (all on Windows).
If users want, they can remap the existing Ctrl + (Shift) + Tab shortcuts to invoke these new commands.

The command goes through all panes (unlike @peterflynn's extension).

cc @peterflynn (you wrote the original extension) @fgimian (you made me do this)

@fgimian
fgimian commented Jun 6, 2015

Thanks @MarcelGerber! 😄

@TomMalbran
Contributor

Nice.

I think that it would be a lot easier to create a new function MainViewManager.traverseToNextViewByListOrder which basically iterates through all the panes, and then through all the files to create an Array.<{file:File, paneId:string}> and get the index of the current file, so that you can later use ViewUtils.traverseViewArray.

@prafulVaishnav
Contributor

@MarcelGerber Verified the changes. Looks good. As @TomMalbran suggested, can you move this to MainViewManager

@MarcelGerber
Member

@prafulVaishnav @TomMalbran Changes pushed. This approach actually looks cleaner to me.

@TomMalbran TomMalbran and 1 other commented on an outdated diff Jun 12, 2015
src/document/DocumentCommandHandlers.js
@@ -1462,9 +1462,28 @@ define(function (require, exports, module) {
}
}
- /** Navigate to the next/previous (MRU) document. Don't update MRU order yet */
- function goNextPrevDoc(inc) {
- var result = MainViewManager.traverseToNextViewByMRU(inc);
+ /**
+ * Finds the next/previous document in MRU or list order
+ * @param {!number} inc Delta indicating in which direction we're going
+ * @param {?boolean} listOrder Whether to navigate using MRU or list order. Defaults to MRU order
+ * @return {?{file:File, paneId:string}} The File object of the next item in the travesal order or null if there aren't any files to traverse.
+ * May return current file if there are no other files to traverse.
+ */
+ function findNextPrevDoc(inc, listOrder) {
@TomMalbran
TomMalbran Jun 12, 2015 Contributor

Do we need this function? It feels like those 4 lines can be added to goNextPrevDoc ?

@MarcelGerber
MarcelGerber Jun 13, 2015 Member

You're right. I just removed it.

@TomMalbran
Contributor

It does look much cleaner now.

@nethip
Contributor
nethip commented Jun 15, 2015

@MarcelGerber This is a very cool addition.

Does it make sense to do minimal refactoring in the core and bundle the rest as an extension?

Also it would be a good idea to chat about this on our Slack channel. That way we would get to know if users want this in the current form or this needs to be tweaked/customized.

@TomMalbran
Contributor

@nethip I don't think that we should make it an extension, not even a core extension. After refactoring the transverse function, the creating of the command would be trivial, so we might end with an extension of just a few lines. But that would increase the loading time unnecessarily.

I read issues from several people saying that the current tabbing using the MRU order is not intuitive, and would like to use a normal order. So this would be good for those users.

@MarcelGerber
Member

@nethip I agree with Tom on this. Notice this is not a behavior change, as the old Ctrl+Tab command still does the same thing as before.
It's just that there's a new command bound to Ctrl+PageUp/PageDown that traverses the list in list order.

There has been some confusion about the order Brackets navigates in, as it seemed like an arbitrary order to some.

It's also what Sublime Text does: They have some shortcut for MRU order and another for list order.

@mackenza
Contributor

definitely agree with @MarcelGerber @TomMalbran This is new functionality, not a change in existing functionality, and belongs in core. Things like this, which are small enhancements to non-extension components (in this case the open files list) should be implemented in core.

I think the test should be "would anyone want this turned off at some point?". If the answer is no, it belongs in core and if yes, it belongs in an extension (even though we don't quite have the ability to turn off default extensions... yet ).

@nethip
Contributor
nethip commented Jun 16, 2015

I am completely OK having this in core.

@TomMalbran I was thinking from the architecture point of view if there is any thing that needs to be considered there, like going forward do we want to place all these kinds of keyboard commands at one place for better maintenance.

I really like the way Visual Studio solves this problem. Upon doing a Ctrl+Tab, it brings up this nice little window like below.

image

Upon releasing Ctrl+Tab, this popup disappears and the doc switch happens. While this window is up, I could use keyboard up and down keyboard keys to choose between what file I want to switch to. IMO this is by far the most elegant solution so far. I was contemplating on adding something like this to Brackets.

From the Brackets context, this window could list the working files as well as list of all recently opened files(which includes the ones not part of the working set as well). That way we could also solve the problem with "No way to browser through list of recently opened files" problem.

@MarcelGerber About the current file switching order broken, did you get a chance to look at the code to figure out why this is in the broken state. Is there is any scope for improvement there?

@MarcelGerber
Member

@nethip To clear things up, I'm not saying the current file switching order is broken.
It's just that people are not used to having a MRU order, and thus don't get the file they want in some cases. That's clearly easier for other editors, as most use a tab-based navigation model, where anything else than list order doesn't make much sense. It's different with Brackets "Working files" list, which doesn't predetermine a particular navigation model.

I like the approach Visual Studio takes, but I'm not really sure whether it makes sense to include two similar-looking, but different lists in there (navigation between them would probably be done with the left/right arrow keys). It might confuse the user.

@nethip
Contributor
nethip commented Jun 17, 2015

@MarcelGerber IMO it makes perfect sense to go VS route on Ctrl-Tab. This is a huge productivity booster for me in VS.

We could probably make it explicit via the UI about working files and the open recent files being listed in the same window. Or chuck out the recent files and stick with just working files, if that makes it really confusing for the users.

About using left/arrow keys in VS, just doing Ctrl-Tab or Ctrl-Shift-Tab is going to traverse forward and backwards respectively, along with a visual feedback on which file is going to be switched to.

The beauty of the way VS handles this is that, the document switch window is sticky and upon doing Ctrl-Tab again, there is a feedback shown to inform the user about the document which is going to be switched to, which the user can override by doing a Ctrl-Tab again.

This kind of solves multiple of our issues/workflows

  • Ability to select the files to intuitively which has a very good visual aspect and hence more confidence on knowing what document the switch is happening to
  • Give a feedback to user about what document it is going to switch to and ability for the user to change the document switch then and there through the same shortcut
  • Ability to put in more widgets/lists e.g. List of opened files which are not in the working set. We could get a little more innovative here
  • All of this being done just by using Ctrl+Tab and Ctrl+Shift+Tab

cc @ryanstewart

@nethip
Contributor
nethip commented Jun 17, 2015

This is a screenshot of how Ctrl-Tab has been implemented in Visual Studio Code.

image

@nethip
Contributor
nethip commented Jun 17, 2015

I am thinking,changes in this PR can co-exist even if we make any changes to the Ctrl-Tab workflow.

@MarcelGerber Just another thing. I was about to merge this PR but then I realized this PR did not have unit tests. Would you mind adding unit tests to this PR? I could then go about merging this.

@MarcelGerber MarcelGerber commented on the diff Jun 17, 2015
src/nls/root/strings.js
@@ -402,6 +402,8 @@ define({
"CMD_CSS_QUICK_EDIT_NEW_RULE" : "New Rule",
"CMD_NEXT_DOC" : "Next Document",
"CMD_PREV_DOC" : "Previous Document",
+ "CMD_NEXT_DOC_LIST_ORDER" : "Next Document (List Order)",
+ "CMD_PREV_DOC_LIST_ORDER" : "Previous Document (List Order)",
@MarcelGerber
MarcelGerber Jun 17, 2015 Member

We should try to make these strings more user-facing and understandable.
Any suggestions?

@MarcelGerber
Member

@nethip I added unit tests.

@nethip nethip commented on an outdated diff Jun 18, 2015
test/spec/MainViewManager-test.js
+ promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: testPath + "/test.html",
+ paneId: "second-pane" });
+ waitsForDone(promise, Commands.FILE_OPEN);
+ });
+ runs(function () {
+ MainViewManager.addToWorkingSet("first-pane", getFileObject("test.js"));
+ MainViewManager.addToWorkingSet("first-pane", getFileObject("test.css"));
+ MainViewManager.addToWorkingSet("second-pane", getFileObject("test.html"));
+ });
+ });
+
+ it("should traverse in list order", function () {
+ runs(function () {
+ MainViewManager.setActivePaneId("first-pane");
+ });
+ runs(function () {
@nethip
nethip Jun 18, 2015 Contributor

This runs is not required as this is already been taken care in the above beforeEach step.

@nethip nethip commented on an outdated diff Jun 18, 2015
test/spec/MainViewManager-test.js
+ runs(function () {
+ CommandManager.execute(Commands.FILE_CLOSE, { file: getFileObject("test.js") });
+ promise = CommandManager.execute(Commands.FILE_OPEN, { fullPath: testPath + "/test.js",
+ paneId: "first-pane" });
+ waitsForDone(promise, Commands.FILE_OPEN);
+ });
+ runs(function () {
+ MainViewManager.setActivePaneId("first-pane");
+
+ var traverseResult = MainViewManager.traverseToNextViewInListOrder(1);
+
+ expect(traverseResult.file).toEqual(getFileObject("test.css"));
+ expect(traverseResult.pane).toEqual("first-pane");
+ });
+ });
+ });
@nethip
nethip Jun 18, 2015 Contributor

MainViewManager.traverseToNextViewInListOrder(-1) missing?

@MarcelGerber
Member

@nethip Ready for another review.

@nethip nethip merged commit 7dea6c9 into master Jun 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcelGerber MarcelGerber deleted the marcel/next-prev-doc-list-order branch Jun 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment