Add Command for navigating around visible editors/viewlets/repl by misoguy · Pull Request #22005 · microsoft/vscode

misoguy · 2017-03-05T10:58:23Z

mention-bot · 2017-03-05T10:58:24Z

@misoguy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bpasero and @egamma to be potential reviewers.

msftclas · 2017-03-05T10:58:28Z

@misoguy,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

misoguy · 2017-03-09T01:47:34Z

@bpasero I've added an abstract class BaseNavigationAction and planning to implement up/down/left/right navigation based on this abstract class. It would be nice if you could take a brief look on this code.
By the way, hygenie seems to fail with no reason...

jtheoof · 2017-03-10T03:35:39Z

The code seems good even though I don't know much of the vscode codebase. I tested it and it works as expected. I was wondering, there is nothing extra to do to be able to bind those commands to custom keybindings right?

bpasero

Seems to work very nicely, some feedback added.

bpasero · 2017-03-10T05:44:57Z

src/vs/workbench/common/editor/editorStacksModel.ts

 	}

-	public next(jumpGroups: boolean): IEditorIdentifier {
+	public next(jumpGroups: boolean, cycleAtEnd = true): IEditorIdentifier {


Please add a test for this in editorStacksModel.test.ts

I'll definitely add this test!

Test added.

bpasero · 2017-03-10T05:45:35Z

src/vs/workbench/electron-browser/main.contribution.ts

 if (isWindows || isLinux) {
 	workbenchActionsRegistry.registerWorkbenchAction(new SyncActionDescriptor(ToggleMenuBarAction, ToggleMenuBarAction.ID, ToggleMenuBarAction.LABEL), 'View: Toggle Menu Bar', viewCategory);
 }
+workbenchActionsRegistry.registerWorkbenchAction(new SyncActionDescriptor(NavigateUpAction, NavigateUpAction.ID, NavigateUpAction.LABEL, null), 'View: Navigate Up', viewCategory);


I wonder if the command name is verbose enough to understand the intent. Can we see how others (VIM?) name this command? Maybe "Navigate to next View to the Left" etc?

I understand that the command name might not be verbose enough.
Do you mean that the View: Navigate Up should be renamed as Navigate to next View to the top or to change the class name itself? Something like class NavigateUp to class NavigateViewToTheTop?

No I was just thinking of the label for users when they find this command in the command palette for example.

Here are Vim's descriptions:

CTRL-W h move to the window on the left CTRL-W j move to the window below CTRL-W k move to the window above CTRL-W l move to the window on the right CTRL-W t move to the TOP window CTRL-W b move to the BOTTOM window

We may want to replace window with View or View Part or similar.

I'll change the label to
'View: Move to the View Part on the left'(right/below/above)
and leave the ID as workbench.action.navigateLeft (as currently implemented)
By the way, is move to the TOP window and move to the BOTTOM window required as well? Since it is currently not implemented...

I would think we should handle those commands in a separate PR if we ever needed them.

bpasero · 2017-03-10T05:51:42Z

src/vs/workbench/electron-browser/actions.ts

+		return this.navigateToLastActiveEditor();
+	}
+
+	private navigateLeftFromEditor(jumpGroups: boolean, isSidebarPositionLeft: boolean): TPromise<boolean> {


Why would we ever not jump groups? I am in a state where I have 2 editors open in one group and navigating left and right jumps between those tabs. My understanding is that this is entirely not about navigating between tabs?

We would not jump groups in 2 situations

When the sidebar is on the left and the current active editor is the first editor in the first group, we would have to go to the sidebar

When the sidebar is on the right, it's the opposite

Sorry, I was not clear: I found a situation where invoking the action was jumping from tab A to tab B in the same single editor group. I suppose that might just be a bug?

As we are just navigating between visible UI parts, we should just navigate between editor groups instead of activating tabs. Otherwise if users open a dozen of files, navigation commands are useless somehow.

If I understand correctly, IEditorGroupService.focusGroup is enough for editor navigation.

@bpasero @rebornix So is it agreed that the navigation should only occur between the View Parts?(ex: sidebar, panel, and 1-3 editor groups)
This means that navigating left and right will not move to the left/right editor of the current group, but will move to the left/right group of the current editor group instead.

@misoguy yes, navigation only between view parts. If there is only one group opened with many editors and no other view part, then basically you cannot navigate anywhere else at all. If more than one group is opened, you can start to navigate between groups.

Understood, this means that the code has to be changed a little bit. I'll get on it when I get the time :)

bpasero · 2017-03-10T05:53:24Z

src/vs/workbench/electron-browser/actions.ts

+	}
+}
+
+export class NavigateDownAction extends BaseNavigationAction {


Should we navigate down to the panel when focus is in the sidebar? Currently that is not happening.

It's my understanding that we will not move down or up when the focus is in the sidebar.
However, if we should navigate down to the panel when focus is in the sidebar, it can be implemented.
Should it be implemented?

Best to talk back to issue creator + VIM guys :)

I'll suggest not to do that at this moment :)

misoguy · 2017-03-10T14:55:50Z

@jtheoof Thanks for testing it out :) However, I didn't really understand what you meant by "I was wondering, there is nothing extra to do to be able to bind those commands to custom keybindings right?". Do you mean is it possible to bind it in a plugin?

misoguy · 2017-03-14T01:59:04Z

Made all changes as requested, and cleared up some unused codes.
I'll remove the [WIP] header in this PR :)
All comments about the PR is welcome!

jtheoof · 2017-03-14T02:24:33Z

@misoguy No I meant if there was anything special to do to have custom keymaps. I do not see the new "commands"

{ "key": "cmd+up",  "command": "navigateUpAction", "when": "editorHasSelection && editorTextFocus" }

would work? (Forgive my noobs question, it's the first time I review a PR in code).

misoguy · 2017-03-14T03:11:24Z

@jtheoof The command you are looking for is workbench.action.navigateUp (or down, right, left depending what you want)
In conclusion, the key mapping you need will be something like below.
{ "key": "cmd+up", "command": "workbench.action.navigateUp" }
("when" is optional)

rebornix · 2017-03-14T03:47:30Z

LGTM, just two tiny comments.

bpasero

@misoguy some more feedback

@Tyriar do we need to whitelist these commands to run them from the integrated terminal? I assigned Cmd+Up to navigate to the view to the top but pressing this in the console did not work but rather printed some weird control characters to the terminal.

bpasero · 2017-03-14T05:40:24Z

src/vs/workbench/electron-browser/main.contribution.ts

 if (isWindows || isLinux) {
 	workbenchActionsRegistry.registerWorkbenchAction(new SyncActionDescriptor(ToggleMenuBarAction, ToggleMenuBarAction.ID, ToggleMenuBarAction.LABEL), 'View: Toggle Menu Bar', viewCategory);
 }
+workbenchActionsRegistry.registerWorkbenchAction(new SyncActionDescriptor(NavigateUpAction, NavigateUpAction.ID, NavigateUpAction.LABEL, null), 'View: Move to the View Part above', viewCategory);


@misoguy please use capital casing here, e.g. View: Move to the View Part Above

bpasero · 2017-03-14T05:41:31Z

src/vs/workbench/electron-browser/actions.ts

+export class NavigateLeftAction extends BaseNavigationAction {
+
+	public static ID = 'workbench.action.navigateLeft';
+	public static LABEL = nls.localize('navigateLeft', "Navigate Left");


@misoguy please make sure that these labels are in sync with where you register them (e.g. this one should be View: Move to the View Part on the Left).

bpasero · 2017-03-14T05:42:36Z

src/vs/workbench/electron-browser/actions.ts

+
+export abstract class BaseNavigationAction extends Action {
+
+	protected NEXT = 'next';


@misoguy a bit weird, why not use an enum outside of the action?

enum Direction { Next, Previous }

misoguy · 2017-03-14T06:51:49Z

@bpasero Changed as requested 👍

bpasero · 2017-03-14T07:31:21Z

Awesome, thanks 👍

Tyriar · 2017-03-14T18:54:29Z

@bpasero yes the whitelist is here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/terminal/electron-browser/terminal.contribution.ts#L145 (messy I know #19770).

cmd+up/down actually seem to be used in Terminal.app to jump to the previous/next command which we may want to implement in the future. The cmd+left/right printing escape sequences may actually be a bug here, opened xtermjs/xterm.js#597

rebornix · 2017-03-14T03:43:41Z

src/vs/workbench/electron-browser/actions.ts

+		super(id, label, groupService, panelService, partService, viewletService);
+	}
+
+	protected navigateOnEditorFocus(isEditorGroupVertical, isSidebarPositionLeft): TPromise<boolean> {


If isEditorGroupVertical is false, navigate left will navigate to the sidebar, whether the sidebar is on the left or right. However, if isEditorGroupVertical is true and isSidebarPositionLeft is false, it won't navigate to the sidebar on the right. The behavior is not consistent.

rebornix · 2017-03-14T03:45:58Z

src/vs/workbench/electron-browser/actions.ts

+	}
+
+	protected navigateOnEditorFocus(isEditorGroupVertical, isSidebarPositionLeft): TPromise<boolean> {
+		if (!isEditorGroupVertical) {


bpasero · 2017-03-16T15:42:18Z

@rebornix I cannot reproduce, I am in this setup and Cmd+Left (for navigating left) does not bring me to the sidebar:

misoguy · 2017-03-17T00:46:41Z

@bpasero There was a bug like @rebornix mentioned but I had it fixed before being merged :)

Add cycle argument to IStacksModelChangeEvent

c831370

msftclas added the cla-already-signed label Mar 5, 2017

bpasero self-assigned this Mar 6, 2017

Add abstract class BaseNavigationAction

d9b64b4

misoguy and others added 7 commits March 10, 2017 08:39

Merge branch 'master' into feature/navigation

a2c3e9b

Change space to pass Hygiene

7a5890c

Add NavigateLeftAction implementation

2125ae6

Add NavigateRightAction implementation

1a54738

Add NavigateUpAction implementation

e281066

Add NavigateDownAction implementation

36c8f69

Add NavigationActions to main.contribution.ts

02f7fff

misoguy mentioned this pull request Mar 10, 2017

VIM: Command for navigating around visible editors/viewlets/repl #21669

Closed

bpasero requested changes Mar 10, 2017

View reviewed changes

sj.hwang added 4 commits March 13, 2017 08:50

Add test for cycle arguments in editorStacksModel

039a54d

Change label for navigating between views

596a7a8

Change navigation actions to move between editor groups

f9316f0

Add navigateAcrossEditorGroup and remove unused fields

44ad2ac

misoguy changed the title ~~[WIP] Add Command for navigating around visible editors/viewlets/repl~~ Add Command for navigating around visible editors/viewlets/repl Mar 14, 2017

bpasero requested changes Mar 14, 2017

View reviewed changes

Fix minor bug and change label

f7dfaf2

bpasero approved these changes Mar 14, 2017

View reviewed changes

bpasero merged commit f54be93 into microsoft:master Mar 14, 2017

bpasero added this to the March 2017 milestone Mar 14, 2017

rebornix mentioned this pull request Mar 14, 2017

Cursor does not go to the end of line when pressing "End" with word wrap enabled #19644

Closed

Tyriar mentioned this pull request Mar 14, 2017

Should cmd+left/right be printing escape sequences? xtermjs/xterm.js#597

Closed

rebornix reviewed Mar 16, 2017

View reviewed changes

github-actions bot locked and limited conversation to collaborators Mar 27, 2020


		export abstract class BaseNavigationAction extends Action {

		protected NEXT = 'next';

Conversation

misoguy commented Mar 5, 2017

Uh oh!

mention-bot commented Mar 5, 2017

Uh oh!

msftclas commented Mar 5, 2017

Uh oh!

misoguy commented Mar 9, 2017

Uh oh!

jtheoof commented Mar 10, 2017

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misoguy Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misoguy commented Mar 10, 2017

Uh oh!

misoguy commented Mar 14, 2017

Uh oh!

jtheoof commented Mar 14, 2017

Uh oh!

misoguy commented Mar 14, 2017

Uh oh!

rebornix commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

misoguy commented Mar 14, 2017

Uh oh!

bpasero commented Mar 14, 2017

misoguy Mar 12, 2017 •

edited

Loading

rebornix commented Mar 14, 2017 •

edited

Loading

bpasero left a comment •

edited

Loading