Add Pinned Tabs #10817

colin-grant-work · 2022-02-28T21:53:44Z

What it does

Commands for pinning and unpinning tabs. Fixes #9326. Closes #9614.

This is based on #9614 with a bug fix and a switch to codicons for pinned tab icons.

How to test

Use the tabbar context menu or command palette to pin and unpin tabs.

In the main or bottom areas, they should receive a pin icon.
If dirty, an editor should receive pin-with-dot icon.
Try to close pinned editors by clicking on the pin icon. They should not close, but should convert to unpinned editors.
Reload the page / restart the application. Pinned state should be restored.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

msujew

I noticed some minor issues in this feature:

Pinning a side bar tab prevents it from closing. Moving this pinned tab to the main area leaves it in an invalid state. The usual close icon is displayed and works as expected. However, the context menu entry for Pin is disabled and the one for Unpin is enabled.
Pinning a focused tab removes the focus from the tab. VSCode refocuses the target tab of the context menu after it closes. This one might not have to do with your PR and is probably more of a general issue of how context menus and tab focus work.

msujew · 2022-03-01T11:57:44Z

packages/core/src/browser/shell/tab-bars.ts

+            h.div({
+                className: 'p-TabBar-tabCloseIcon',
+                onclick: this.handleCloseClickEvent
+            })


Minor: This item is missing its hovering effect. You can simply add the action-item class to enable it.

Thanks. I think the original PR predated your work in that area.

msujew · 2022-03-01T12:04:22Z

packages/core/src/browser/common-frontend-contribution.ts

+    export const PIN_TAB = Command.toDefaultLocalizedCommand({
+        id: 'workbench.action.pinEditor',
+        category: VIEW_CATEGORY,
+        label: 'Pin Tab'
+    });
+    export const UNPIN_TAB = Command.toDefaultLocalizedCommand({
+        id: 'workbench.action.unpinEditor',
+        category: VIEW_CATEGORY,
+        label: 'Unpin Tab'
+    });


The eslint rule doesn't really work for toDefaultLocalizedCommand (I should fix this perhaps)

Pin and Unpin are the labels used by VSCode and can both be found in the nls.metadata.json file.

I did wonder about whether the linting would catch it for the other NLS utilities. I'll update the text.

I actually just noticed that VSCode is using Pin editor/Unpin editor. Since we do not only allow pinning on editors, we should maybe introduce new translations here? Or is Pin editor accurate enough for the command label?

I think editors are going to be the most common targets, in any case, so I'll use '(un)pin editor' for now.

colin-grant-work · 2022-03-01T16:10:56Z

I noticed some minor issues in this feature:

Pinning a side bar tab prevents it from closing. Moving this pinned tab to the main area leaves it in an invalid state. The usual close icon is displayed and works as expected. However, the context menu entry for Pin is disabled and the one for Unpin is enabled.

Pinning a focused tab removes the focus from the tab. VSCode refocuses the target tab of the context menu after it closes. This one might not have to do with your PR and is probably more of a general issue of how context menus and tab focus work.

@msujew, thanks for the review. Do you remember which views had problems in this scenario? It seems some widgets are OK (e.g. editors) and others unpin themselves (e.g. extensions). If you remember which ones you had trouble with, I'll be sure to test them and figure out what's going wrong.

colin-grant-work · 2022-03-01T17:32:18Z

@msujew, I think the problem was probably code in ViewContainer, and I've adjusted that, and adjusted the code checking enablement of the commands to consistently check the closeable field, as well, so that closeable: true is always treated as pinned: false even if the classname is present.

packages/core/src/browser/common-frontend-contribution.ts

msujew · 2022-03-02T12:20:54Z

packages/core/src/browser/common-frontend-contribution.ts

+            },
+            {
+                command: CommonCommands.UNPIN_TAB.id,
+                keybinding: 'ctrlcmd+k shift+enter',


I noticed that pinning does work using the specified keyboard shortcut. However, unpinning does not work for some reason.

It turns out this was also a consequence of the 'editor' vs. 'widget' question. An earlier version of the PR had either had two context keys or only activeWidgetIsPinned and it was switched to editor to agree with VSCode, but the keybindings still referred to widget.

Add icons for pinned tabs Add keybindings for pinning and unpinning tabs Add a TODO notice for icons Add activeEditorIsPinned context key Add activeWidgetIsPinned context key while keeping vscode compatibility Change pin/unpin commands to use activeWidgetIsPinned Use VS Code command IDs for pin/unpin Create TheiaTitle type Refactor checks for pinning Make preview editor non-preview when pinned Unpin tab by clicking on icon Fix reordering when unpinning Do not rearrange tabs when pinned/unpinned Make TheiaTitle a generic type Co-authored-by: Colin Grant <[email protected]> Co-authored-by: Dmitry Sharshakov <[email protected]>

msujew

Alright, I noticed a minor issue with the keyboard shortcut: When an editor is unfocused (for example by clicking on the tab-bar), using the shortcut to pin works, but using it to unpin does not. It works in every other case that I've tested. Since this is quite minor, I will approve this PR nonetheless, as everything else is working.

Pinning/Unpinning editors using the Command/Context Menu works correctly
Also works mostly correctly with the shortcut
It also works as expected for other widgets, like Search and Debug, even when the widget is on the sidebar
The context menu entries related to closing files are correctly disabled depending on which widgets are pinned.

colin-grant-work · 2022-03-07T17:19:56Z

Alright, I noticed a minor issue with the keyboard shortcut: When an editor is unfocused (for example by clicking on the tab-bar), using the shortcut to pin works, but using it to unpin does not. It works in every other case that I've tested. Since this is quite minor, I will approve this PR nonetheless, as everything else is working.

I appreciate the thorough testing! I will check on this scenario.

vince-fugnitto

I confirmed the following 👍

pinning/unpinning works
pinning/unpinning tabs in the main-area properly decorate the tab with the pin icon
pinning a editor-preview works, the editor becomes persistent
pinning dirty editors works, the tab is decorated with the dirty pin icon
pinning tabs in the sidepanels work well
pinned tabs are uncloseable unless unpinned first
pinned tabs are persistent on reloads
pinning with the keyboard shortcut works well
unpinning with the keyboard shortcut does not seem to work for some widgets (ex: reproducible with search-in-workspace) (likely what Mark noticed too)

…widget

colin-grant-work · 2022-03-07T18:41:45Z

@msujew, @vince-fugnitto, thanks again for the thorough reviews. I believe I tracked down the problem with unpinning unfocused widgets: the context key setting and the command enablement were looking at different conditions. I have adjusted the code so that the context key is always looking at the same widget as the command will act on.

vince-fugnitto

I confirmed that the outstanding issue regarding the keyboard shortcut has been fixed, the last issue in #10817 (review) now works 👍

msujew

Thanks Colin, that last change seems to have done it 👍

colin-grant-work added the shell issues related to the core shell label Feb 28, 2022

msujew self-requested a review March 1, 2022 10:45

msujew reviewed Mar 1, 2022

View reviewed changes

colin-grant-work force-pushed the finish-up/pinned-tabs branch 2 times, most recently from 4e6dc30 to 8e1fdaa Compare March 1, 2022 17:30

colin-grant-work force-pushed the finish-up/pinned-tabs branch 2 times, most recently from f39b1f7 to c17ac0e Compare March 1, 2022 19:24

msujew reviewed Mar 2, 2022

View reviewed changes

dsseng and others added 5 commits March 7, 2022 09:39

fix side panel restoration

be2eb0b

switch to codicons

283ae3e

ensure view containers don't unpin themselves

0d24681

nls issues

223c372

colin-grant-work force-pushed the finish-up/pinned-tabs branch from 2d24776 to 9f45e11 Compare March 7, 2022 16:41

Use correct context key

eef3aeb

colin-grant-work force-pushed the finish-up/pinned-tabs branch from 9f45e11 to eef3aeb Compare March 7, 2022 17:09

msujew approved these changes Mar 7, 2022

View reviewed changes

vince-fugnitto reviewed Mar 7, 2022

View reviewed changes

extract utilities; ensure context key and command use same reference …

fe4cdf9

…widget

vince-fugnitto approved these changes Mar 7, 2022

View reviewed changes

msujew approved these changes Mar 8, 2022

View reviewed changes

colin-grant-work merged commit 334b92e into eclipse-theia:master Mar 8, 2022

colin-grant-work deleted the finish-up/pinned-tabs branch March 8, 2022 22:09

colin-grant-work added this to the 1.24.0 milestone Mar 8, 2022

msujew mentioned this pull request Jun 9, 2022

Add tooltip for tab close button #11272

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pinned Tabs #10817

Add Pinned Tabs #10817

colin-grant-work commented Feb 28, 2022 •

edited

Loading

msujew left a comment

msujew Mar 1, 2022

colin-grant-work Mar 1, 2022

msujew Mar 1, 2022

colin-grant-work Mar 1, 2022

msujew Mar 2, 2022

colin-grant-work Mar 3, 2022

colin-grant-work commented Mar 1, 2022

colin-grant-work commented Mar 1, 2022

msujew Mar 2, 2022

colin-grant-work Mar 3, 2022

msujew left a comment

colin-grant-work commented Mar 7, 2022

vince-fugnitto left a comment

colin-grant-work commented Mar 7, 2022

vince-fugnitto left a comment

msujew left a comment

Add Pinned Tabs #10817

Add Pinned Tabs #10817

Conversation

colin-grant-work commented Feb 28, 2022 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

msujew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 1, 2022

colin-grant-work commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 7, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

colin-grant-work commented Mar 7, 2022

vince-fugnitto left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

colin-grant-work commented Feb 28, 2022 •

edited

Loading