Implement join editors command by initialshl · Pull Request #22389 · microsoft/vscode

initialshl · 2017-03-10T12:44:56Z

Fixes #20279

This command joins the editor group with the previous group (except for the first group which joins with the next group).

The order of editors in the joining group is preserved when joining with the target group.

If an editor is opened in both the joining and target group, only one will be kept, in the same position as in the joining group.

mention-bot · 2017-03-10T12:44:58Z

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

msftclas · 2017-03-10T12:45:00Z

@initialshl,
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

bpasero

Good start, added some feedback.

This makes me think: do we want more commands that allow to "Split Editors"? Like the inverse operation of "Join Editors" which would basically mean to start a new group starting with the current active editor and move all editors to the right to a new group. Would be pretty cool :)

bpasero · 2017-03-10T14:23:11Z

src/vs/workbench/browser/parts/editor/editor.contribution.ts

 registry.registerWorkbenchAction(new SyncActionDescriptor(CloseOtherEditorsInGroupAction, CloseOtherEditorsInGroupAction.ID, CloseOtherEditorsInGroupAction.LABEL, { primary: null, mac: { primary: KeyMod.CtrlCmd | KeyMod.Alt | KeyCode.KEY_T } }), 'View: Close Other Editors', category);
 registry.registerWorkbenchAction(new SyncActionDescriptor(CloseEditorsInOtherGroupsAction, CloseEditorsInOtherGroupsAction.ID, CloseEditorsInOtherGroupsAction.LABEL), 'View: Close Editors in Other Groups', category);
 registry.registerWorkbenchAction(new SyncActionDescriptor(SplitEditorAction, SplitEditorAction.ID, SplitEditorAction.LABEL, { primary: KeyMod.CtrlCmd | KeyCode.US_BACKSLASH }), 'View: Split Editor', category);
+registry.registerWorkbenchAction(new SyncActionDescriptor(JoinEditorsAction, JoinEditorsAction.ID, JoinEditorsAction.LABEL), 'View: Join Editors', category);


Should this maybe be more descriptive to what it actually does? Join Editors of two Groups?

bpasero · 2017-03-10T14:23:57Z

src/vs/workbench/browser/parts/editor/editorActions.ts

+		@IWorkbenchEditorService private editorService: IWorkbenchEditorService,
+		@IEditorGroupService private editorGroupService: IEditorGroupService
+	) {
+		super(id, label, 'join-editors-action');


You do not need the CSS class here unless it would show up as icon, so I suggest to remove it.

bpasero · 2017-03-10T14:26:16Z

src/vs/workbench/browser/parts/editor/editorActions.ts

+	}
+
+	public run(context?: IEditorContext): TPromise<any> {
+		let editorToJoin: IEditor;


It looks like you only need position and input from the IEditor. If so, I suggest to completley bypass the editorService API and do all of this from the stacks model that provides all you need (inputs, groups, a way to convert from a group to a position) from editor group service (there is a bit of overlap for historic reasons).

The stacks model is really the truth of what is going up in the 1-3 editor groups and you should be able to do everything with it. The more heavy IEditor is the actual instance of control that hosts inputs.

bpasero · 2017-03-10T14:35:04Z

src/vs/workbench/browser/parts/editor/editorActions.ts

+
+		// If an editor exists in joining group and target group, only the editor in the joining group is kept.
+		if (targetPosition < toJoinPosition) {
+			toJoinEditors.forEach(e => this.editorGroupService.moveEditor(e, toJoinPosition, targetPosition, targetGroup.count));


This is a bit crazy to do because for each editor that gets moved it will be opened in the target group causing an editor switch. It would be much smoother if the move would happen with the editors opening inactive on the target group and only the last one would actually be active. There is an option that can be set when opening an editor called inactive that I think we should wire into the moveEditor call so that we can optimize for this (for the openEditor call here).

initialshl · 2017-03-11T04:16:10Z

@bpasero Thanks for the review!
I have refactored some parts and also used the inactive option, and added the new gif in the description of this PR.

do we want more commands that allow to "Split Editors"?

Indeed, Split Editors would be a useful and convenient command, I can see myself using it a lot!

bpasero

Awesome, just a minor comment left.

bpasero · 2017-03-11T07:14:11Z

src/vs/workbench/browser/parts/editor/editorPart.ts

-	public moveEditor(input: EditorInput, from: EditorGroup, to: EditorGroup, index?: number): void;
-	public moveEditor(input: EditorInput, from: Position, to: Position, index?: number): void;
-	public moveEditor(input: EditorInput, arg2: any, arg3: any, index?: number): void {
+	public moveEditor(input: EditorInput, from: EditorGroup, to: EditorGroup, index?: number, inactive?: boolean): void;


I suggest to put index and inactive into a new optional options bag (IMoveOptions).

bpasero · 2017-03-11T07:14:32Z

src/vs/workbench/services/group/common/groupService.ts

 	 */
-	moveEditor(input: IEditorInput, from: IEditorGroup, to: IEditorGroup, index?: number): void;
-	moveEditor(input: IEditorInput, from: Position, to: Position, index?: number): void;
+	moveEditor(input: IEditorInput, from: IEditorGroup, to: IEditorGroup, index?: number, inactive?: boolean): void;


IMoveOptions

bpasero

more feedback, almost there 👍

bpasero · 2017-03-11T13:56:12Z

src/vs/workbench/browser/parts/editor/editorActions.ts

+		const fromGroupTotalCount = fromGroupEditors.length;
+
+		// If an editor exists in both groups, only the editor in the joining group is kept
+		if (toPosition < fromPosition) {


We still need to improve this: Now we start to move editors from the group to another group starting with the first editor of that group, going to the last. This is bad when the first editor of the group is also the active editor because after each editor is moved, the group will show the next editor as active.

As such, please first move all the inactive editors of the group and only at last the active editor. This will again prevent many unwanted editor change events being fired.

…oupsAction

initialshl · 2017-03-12T06:42:41Z

@bpasero Thanks for the feedbacks. I overlooked the inactive option which can be used to control the switching of the active editor. It appears that the preserveFocus option has to be set too, so that the toGroup is activated only after the active editor has moved.

And regarding the possibility of adding a new command SplitEditors, I would be happy to contribute another PR for that if it is good 👍

bpasero · 2017-03-12T07:23:18Z

@initialshl yes, something like "View: Split Editors into Two Groups" 👍 . Ideally we could have a common base class of both actions to share some logic for both.

This PR looks nice now, thanks. I will merge it in tomorrow.

kounelios13 · 2018-01-21T09:39:55Z

is there any key combination available?

bpasero · 2018-01-21T11:44:54Z

@kounelios13 just assign it via keybindings settings:

kounelios13 · 2018-01-21T12:10:48Z

Hello.I've noticed something.If I have more than 2 editors open when I execute this command it won't join all of them.Is there any way to achieve this behavior?

bpasero · 2018-01-21T12:45:36Z

Works for me, even with 3 editor groups open.

kounelios13 · 2018-01-21T12:58:20Z

@bpasero If I have 3 editors open it will merge two of them and then it leaves me with 2 editors open

bpasero · 2018-01-21T13:17:15Z

@kounelios13 what would you expect to happen?

kounelios13 · 2018-01-21T13:40:01Z

@bpasero I would expect that if I have let's say 5 editors open after executing the join command it would merge them all into 1.

Isn't that the purpose of this command or have I misunderstood it?

bpasero · 2018-01-21T15:49:33Z

@kounelios13 as the name of that action implies, it will only merge the editors of 2 groups, not all groups.

kounelios13 · 2018-01-21T19:54:49Z

Sorry my mistake.

Implement join editors command

034bfcb

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

bpasero requested changes Mar 10, 2017

View reviewed changes

ramya-rao-a assigned bpasero Mar 10, 2017

initialshl added 2 commits March 11, 2017 11:57

Allow inactive option when moving editors across groups

522c6e0

Rename JoinEditorsAction to JoinTwoGroupsAction with code refactoring

da2d664

bpasero requested changes Mar 11, 2017

View reviewed changes

Add option bag IMoveOptions for move editor methods

cc3d737

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

bpasero requested changes Mar 11, 2017

View reviewed changes

Prevent unnecessary switchings of active editor or group in JoinTwoGr…

791341b

…oupsAction

bpasero approved these changes Mar 13, 2017

View reviewed changes

bpasero merged commit dd197f1 into microsoft:master Mar 13, 2017

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

Conversation

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

Uh oh!

Uh oh!

mention-bot commented Mar 10, 2017

Uh oh!

msftclas commented Mar 10, 2017

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 10, 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!

bpasero Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

initialshl commented Mar 11, 2017

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kounelios13 commented Jan 21, 2018

Uh oh!

bpasero commented Jan 21, 2018

Uh oh!

kounelios13 commented Jan 21, 2018

Uh oh!

bpasero commented Jan 21, 2018

Uh oh!

kounelios13 commented Jan 21, 2018

Uh oh!

bpasero commented Jan 21, 2018

Uh oh!

kounelios13 commented Jan 21, 2018

Uh oh!

bpasero commented Jan 21, 2018

Uh oh!

kounelios13 commented Jan 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

initialshl commented Mar 10, 2017 •

edited

Loading

bpasero Mar 10, 2017 •

edited

Loading

initialshl commented Mar 12, 2017 •

edited

Loading

bpasero commented Mar 12, 2017 •

edited

Loading