Add setting to prevent closing the pinned tab when using middle click (fix #115734)

[Kalmaegi]

Fix #115734

[Kalmaegi]

@bpasero Hi, I have two questions:
Do we need to make settings for this obstacle?
If I add judgment for pinned here, the scope may be a little larger. Do we need to move the judgment to where the mouse middle button is clicked to determine, in order to reduce the impact scope of this obstacle?
Thanks!

[bpasero]

@weartist I would have a setting which then we can turn on by default to see how many users are upset. If there are no complaints I am also easy to remove the setting again and make it the default behaviour.

I would think the change should only be where we close from mouse middle click:

For the "Open Editors" view:

vscode/src/vs/workbench/contrib/files/browser/views/openEditorsView.ts

Line 271 in 5e0c49c

e.element.group.closeEditor(e.element.editor, { preserveFocus: true });

For tabs:

vscode/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

Line 891 in 5e0c49c

this.closeEditorAction.run({ groupId: this.group.id, editorIndex: index });

For no tabs:

vscode/src/vs/workbench/browser/parts/editor/noTabsTitleControl.ts

Line 108 in 5e0c49c

this.group.closeEditor(this.group.activeEditor);

[Kalmaegi]

@weartist I would have a setting which then we can turn on by default to see how many users are upset. If there are no complaints I am also easy to remove the setting again and make it the default behaviour.

I would think the change should only be where we close from mouse middle click:

For the "Open Editors" view:

vscode/src/vs/workbench/contrib/files/browser/views/openEditorsView.ts

Line 271 in 5e0c49c

e.element.group.closeEditor(e.element.editor, { preserveFocus: true });

For tabs:

vscode/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts

Line 891 in 5e0c49c

this.closeEditorAction.run({ groupId: this.group.id, editorIndex: index });

For no tabs:

vscode/src/vs/workbench/browser/parts/editor/noTabsTitleControl.ts

Line 108 in 5e0c49c

this.group.closeEditor(this.group.activeEditor);

Understand, i rushed XD, and thank you for the code point!

[Kalmaegi]

I simply completed the feature and tested it, hoping it worked fine @bpasero

[bpasero]

The method is actually called isSticky in the internal API, I know its misleading, but isPinned is for a different feature (preview tabs).

Should the setting be more specific to mention that it is for mouse middle click only? We already prevent closing a tab by keybinding irrespective of the setting.

[Kalmaegi]

The method is actually called isSticky in the internal API, I know its misleading, but isPinned is for a different feature (preview tabs).

Should the setting be more specific to mention that it is for mouse middle click only? We already prevent closing a tab by keybinding irrespective of the setting.

That is, can I replace isPinned with isSticky? I did confuse the two judgments a bit, I found that the two were almost identical, then I found their implementation, but still not possible to confirm whether they are equal haha

isPinned:

vscode/src/vs/workbench/common/editor/editorGroupModel.ts

Line 720 in 885dba3

isPinned(editorOrIndex: EditorInput | number): boolean {

isSticky:

vscode/src/vs/workbench/common/editor/editorGroupModel.ts

Line 720 in 885dba3

isPinned(editorOrIndex: EditorInput | number): boolean {

and I'll go and change the description again

[bpasero]

Yes, can be replaced with isSticky.

[bpasero]

@weartist I had a thought: what if the setting changes back to the original name you had in mind and becomes an enumeration that allows to fully control the behaviour for closing:

• one option to never close for neither keyboard nor mouse
• one option to never close for keyboard
• one option to never close for mouse
• one option to always allow to close

This would be a good way to keep the setting around even for the future to let people fully control the close behaviour and then we can think about a reasonable default?

[Kalmaegi]

@weartist I had a thought: what if the setting changes back to the original name you had in mind and becomes an enumeration that allows to fully control the behaviour for closing:

• one option to never close for neither keyboard nor mouse
• one option to never close for keyboard
• one option to never close for mouse
• one option to always allow to close

This would be a good way to keep the setting around even for the future to let people fully control the close behaviour and then we can think about a reasonable default?

@bpasero Does your mention of ' never close for mouse' include right-clicking the tab and selecting 'close' from the pop-up menu? I think it's well that this feature can be fully controlled through settings. For me, I need to prevent the keyboard from closing tabs, but I still want to be able to close them by right-clicking with the mouse, otherwise I'd have to cancel pinned and click again every time.

[bpasero]

Yeah I was suggesting to target only these 2 cases:

• closing by keybinding
• closing by mouse

But not other actions, such as the menu.

[Kalmaegi]

In this case, does mouse closing only include clicking the mouse middle button? I will try to write a simple version.

[Kalmaegi]

will start tomorrow. Tonight I will think about whether there are some other cases to consider XD

[bpasero]

Yeah, so I meant middle-mouse-click.

[Kalmaegi]

@bpasero I want to abstract the logic that judges whether to block closing into a method, something like 'isPinned', but I didn't find a good place for it. The current code has some repetition and verbosity

[bpasero]

Yeah some code-reuse might be good, for example from a method in src/vs/workbench/common/editor.ts. I feel the method could accept the editor in question and then either the configuration service or the part options to return if the editor should close based on a context such as mouse or key.

[Kalmaegi]

have reworked the relevant code, but have not tested it yet

[Kalmaegi]

After a simple test, the function looks fine

[bpasero]

@weartist I did a review and pushed changes, can you please test out everything works as expected?

[Kalmaegi]

@weartist I did a review and pushed changes, can you please test out everything works as expected?

sure, thanks! I'll start testing after reading it, this is my favorite part to learn in order for me to improve haha

[Kalmaegi]

@bpasero It's run as excepted