Add ability to pin tabs #9614

dsseng · 2021-06-20T08:24:54Z

What it does

Commands for pinning and unpinning tabs. Fixes #9326.

How to test

Open multiple editors, pin and unpin some of those

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

vince-fugnitto

@sh7dm are you planning on updating the UI for pinned tabs as well, to visually represent that they are pinned? At the moment there is no distinction between pinned tabs and regular tabs.

For example, in the issue and in vscode we have (first tab is pinned):

dsseng · 2021-06-21T12:32:08Z

@vince-fugnitto hello, I plan for sure. Is title.className the correct way to add extra classes to Phosphor tabs?

vince-fugnitto · 2021-06-21T12:34:43Z

@sh7dm I believe it would be a fine solution, for example it is used when setting a dirty decoration in editors:

theia/packages/core/src/browser/saveable.ts

Lines 207 to 217 in 4c9b8b8

    
           /** 
        
            * The class name added to the dirty widget's title. 
        
            */ 
        
           const DIRTY_CLASS = 'theia-mod-dirty'; 
        
           export function setDirty(widget: Widget, dirty: boolean): void { 
        
               const dirtyClass = ` ${DIRTY_CLASS}`; 
        
               widget.title.className = widget.title.className.replace(dirtyClass, ''); 
        
               if (dirty) { 
        
                   widget.title.className += dirtyClass; 
        
               } 
        
           }

dsseng · 2021-06-21T13:12:36Z

One more feature: keyboard shortcut. Is it that simple like registering both on the same shortcuts since those commands are mutually exclusive?

vince-fugnitto · 2021-06-21T13:59:53Z

One more feature: keyboard shortcut. Is it that simple like registering both on the same shortcuts since those commands are mutually exclusive?

It should be, you can try (and set it to the same value found in vscode).

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

dsseng · 2021-06-21T14:10:53Z

One more feature: keyboard shortcut. Is it that simple like registering both on the same shortcuts since those commands are mutually exclusive?

It should be, you can try (and set it to the same value found in vscode).

No, that didn't work :( . I changed unpin to ctrlcmd+k ctrlcmd+shift+enter and both worked. We can do it as a single pin-unpin command

dsseng · 2021-06-21T17:33:35Z

So, I think I can merge those two commands into one to save space in menu + give users a single keybinding?

vince-fugnitto · 2021-06-21T18:09:25Z

So, I think I can merge those two commands into one to save space in menu + give users a single keybinding?

@sh7dm I'm not sure I follow? In vscode there are two commands present:

export const PIN_EDITOR_COMMAND_ID = 'workbench.action.pinEditor';
export const UNPIN_EDITOR_COMMAND_ID = 'workbench.action.unpinEditor';

I do not think we should merge the commands, rather their enablement should be controlled by if the editor is currently pinned or not.

dsseng · 2021-06-21T18:14:47Z

Sadly, those couldn't coexist with the same keybinding (see my comment)

vince-fugnitto · 2021-06-21T18:23:04Z

Sadly, those couldn't coexist with the same keybinding (see my comment)

They can, just need a proper when context:

vince-fugnitto

A couple of observations with the pull-request:

Should the functionality be specific to editors-widgets only?
What behavior do we expect for pinned editors in the sidebar (not main-area), they are currently not properly decorated.
Clicking the pin icon of a pinned editor does not unpin it like in VS Code, any plans to align?
Pinning multiple views in the sidebar re-arranges them in an order that it is unexpected.

In general, perhaps we can simplify to only pinning editors, and only pinning them in the main-area.

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

dsseng · 2021-06-23T16:17:01Z

I did not really notice weird ordering in sidebar. @vince-fugnitto could you please describe reproduction steps?

vince-fugnitto · 2021-06-23T16:50:02Z

I did not really notice weird ordering in sidebar. @vince-fugnitto could you please describe reproduction steps?

I could no longer reproduce it, I believe part of the confusion is due to side-panels not being decorated when pinned.

dsseng · 2021-06-23T17:18:08Z

So how should we decorate pinned sidebar widgets? There seems to be quite a lot of free space in the corner where SVG pin icon can be placed. Top-right should be great. What do you think on that (at least for this PR)?

Or some sort of badge like SCM has...

vince-fugnitto · 2021-06-23T18:02:51Z

So how should we decorate pinned sidebar widgets?

@sh7dm I'm not really sure how it should look, we already have some issues in sidepanels for editors when changing file-icon themes. I would not hold back the pull-request if the initial changes does not add decorations. @colin-grant-work any thoughts?

colin-grant-work

This is looking pretty good to me. One small point is that in VSCode, when you pin an editor preview widget, it converts to a normal widget, but that isn't happening here.

If you'd like to implement that for editor previews, you can modify the initializePreview method in editor-preview-widget.ts to look like this:

    initializePreview(): void {
        const oneTimeListeners = new DisposableCollection();
        this._isPreview = true;
        this.title.className += ` ${PREVIEW_TITLE_CLASS}`;
        const oneTimeDirtyChangeListener = this.saveable.onDirtyChanged(() => {
            this.convertToNonPreview();
            oneTimeListeners.dispose();
        });
        oneTimeListeners.push(oneTimeDirtyChangeListener);
        const oneTimeTitleChangeHandler = () => {
            if ((this.title as any).pinned) {
                this.convertToNonPreview();
                oneTimeListeners.dispose();
            }
        };
        this.title.changed.connect(oneTimeTitleChangeHandler);
        oneTimeListeners.push(Disposable.create(() => this.title.changed.disconnect(oneTimeTitleChangeHandler)));
        this.toDispose.push(oneTimeListeners);
    }

If you're interested in implementing the unpin-on-click behavior, one way to do it would be to modify the rendering here to react to pinned / unpinned status. Another strategy would be do something like this:

theia/packages/editor-preview/src/browser/editor-preview-manager.ts

Line 70 in 74fbeb7

    
           document.addEventListener('dblclick', this.convertEditorOnDoubleClick.bind(this));

Where you check whether the click is on an element with the class name you're interested in and, if so, execute the unpin command. But I'm fine leaving that out, for now.

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

colin-grant-work · 2021-06-23T19:51:33Z

So how should we decorate pinned sidebar widgets?

@sh7dm I'm not really sure how it should look, we already have some issues in sidepanels for editors when changing file-icon themes. I would not hold back the pull-request if the initial changes does not add decorations. @colin-grant-work any thoughts?

I agree with this. Figuring out how to handle the decoration on sidebars will be a bit tricky, but I also think it isn't urgent because pinning in that context is going to be much less common than pinning in the main area, so we don't have to resolve it immediately.

dsseng · 2021-06-24T08:18:36Z

Build failure is not caused by changes, there's some network or server error. https://github.com/eclipse-theia/theia/runs/2902667623#step:5:1446

colin-grant-work · 2021-06-24T16:33:52Z

@sh7dm this is working very well for me. I'm not seeing any problems on the main tabbar or other tabbars. Personally, I don't really like the tab rearrangement behavior, but that is how VSCode is doing it, and this code implements it correctly.

On the code side, I think I'm going to backpedal, with apologies, and suggest that the TheiaTitle type, while necessary in the long run, should maybe be created in a separate refactor where we apply it throughout the application. As long as we have to do a bunch of casts, it doesn't much matter whether it's to any or TheiaTitle, and introducing it here has actually increased the number of casts, rather than reducing them. Sorry to change directions there!

Otherwise the code looks good to me. @vince-fugnitto, are you comfortable with the changes to the tabbar code introduced by this commit? They look reasonable to me, but it's digging pretty deep into core.

dsseng · 2021-06-24T17:24:32Z

Personally, I don't really like the tab rearrangement behavior, but that is how VSCode is doing it, and this code implements it correctly.

Why clone VSCode behavior in terms of UX? Theia is a different project with its own UX, we can implement better behavior I think. If possible, suggest your ideas for rearrangement.

As long as we have to do a bunch of casts, it doesn't much matter whether it's to any or TheiaTitle, and introducing it here has actually increased the number of casts, rather than reducing them. Sorry to change directions there!

any cast is a linter error we have to suppress, so type is better, but I'm fine with casting to any to limit scope of the PR.

colin-grant-work · 2021-06-24T17:43:18Z

Why clone VSCode behavior in terms of UX? Theia is a different project with its own UX, we can implement better behavior I think. If possible, suggest your ideas for rearrangement.

A fair question. The answer (not a very good one) is that it's at least a baseline to target, and users familiar with VSCode will have a lower learning / surprise curve if we stick closely to the patterns VSCode establishes. I suppose the arguments run as follows:

For rearranging

If you pin an editor, you probably want to keep it visible, and moving it to the left sort of achieves that.

But... probably isn't definitely, and 'left' doesn't mean 'visible' unless you fix the scroll behavior somehow so that you can't scroll them out of view.

Against rearranging

It's visually a bit disorienting to see your tab zoom away, and if you have a lot tabs and decide to pin one that's far to the right, you can actually zoom it out of view.
In VSCode, it looks like pinning and location are actually mutually dependent. If you move a pinned tab to the right of an unpinned tab, it unpins. That's pretty unintuitive to me, though I'm sure I could get used to it.

That's a specific quirk that doesn't have to do with the question of rearrangement in general.

My preference would be to just decouple pinning and arrangement, since they're conceptually separate. You can pin a tab if you don't want it to close (I'm a habitual ctrl+k+w user, so that appeals to me), but you can then put that pinned tab wherever you want (why not?). Alternatively, we could implement an application preference 'application.pinnedTabsLeft' or something that determines whether to rearrange or not. Then everyone (except you, the developer of the feature 😉) is happy.

any cast is a linter error we have to suppress, so type is better, but I'm fine with casting to any to limit scope of the PR.

True, and I won't insist on reverting to any if you prefer to start the move to a TheiaTitle type (and @vince-fugnitto agrees?), though I would probably make it generic (with a default, since Widget is by far our most common Title type) TheiaTitle<T = Widget> extends Title<T>.

vince-fugnitto

@sh7dm do we plan on supporting the following use-case? (supported in vscode)

pin an editor
reload the application
the editor is restored, but not its pinned state

dsseng · 2021-07-19T13:28:36Z

I'd like to do so. How to include some extra data to save of application state?

vince-fugnitto · 2021-07-19T13:55:18Z

I'd like to do so. How to include some extra data to save of application state?

In order to store and restore state, StatefulWidget needs to be implemented.
You'll likely need to look at implementations and attempt to restore the pinned state.

colin-grant-work · 2021-07-19T14:03:55Z

In order to store and restore state, StatefulWidget needs to be implemented.

Perhaps this is a case where the ApplicationShell should do the work, since all of the pinning logic resides there, rather than on the widget? Otherwise all pinnable widgets (all widgets) would need to become Stateful, and that could wreck a lot of existing StatefulWidget code.

vince-fugnitto · 2021-07-19T14:10:03Z

In order to store and restore state, StatefulWidget needs to be implemented.

Perhaps this is a case where the ApplicationShell should do the work, since all of the pinning logic resides there, rather than on the widget? Otherwise all pinnable widgets (all widgets) would need to become Stateful, and that could wreck a lot of existing StatefulWidget code.

You're right, we should save layout and not widget state. So we should take a look at setLayoutData, and restoreLayout.

dsseng · 2021-07-19T14:28:38Z

Thanks, will do!

Signed-off-by: Dmitry Sharshakov <[email protected]>

vince-fugnitto

Please be sure to clean up the commits, it seems the ECA check is failing.

packages/core/src/browser/shell/application-shell.ts

dsseng · 2021-07-20T13:21:47Z

I fixed ECA by going to the website, now it's all-ok.

Signed-off-by: Dmitry Sharshakov <[email protected]>

vince-fugnitto

I'm not sure about how you're restoring the pinned state on reload, mainly about why its only being done to main and bottom and not in a general way, I'll need some time to think about it.

@colin-grant-work I'd appreciate if you can take a look and provide any feedback if you have any.

dsseng · 2021-07-26T17:03:47Z

@vince-fugnitto Side panels are done in their generic code, but I couldn't find a good generic place for it in main/bottom.

colin-grant-work · 2021-07-30T19:18:00Z

packages/core/src/browser/shell/application-shell.ts

+        const pinned: boolean[] = [];
+
+        toArray(this.mainPanel.widgets()).forEach((a, i) => {
+            pinned[i] = a.title.className.indexOf('theia-mod-pinned') >= 0
+        });


maybe just this:

const pinned = toArray(this.mainPanel.widgets()).map(widget => widget.title.className.includes('theia-mod-pinned');

Alternatively, since there's some uncertainty below about whether the number of widgets examined here will be the same as the number of widgets eventually restored, you could use something like WidgetManager.getDescription() to get a (probably) unique string(ifiable) identifier for each widget, use that as the key in a Record<string, boolean> and then match that up with the restored widgets on the other side of the restoration.

colin-grant-work · 2021-07-30T19:21:17Z

packages/core/src/browser/shell/application-shell.ts

@@ -638,6 +645,14 @@ export class ApplicationShell extends Widget {
        if (mainPanel) {
            this.mainPanel.restoreLayout(mainPanel);
            this.registerWithFocusTracker(mainPanel.main);
+            const widgets = toArray(this.mainPanel.widgets());
+            if (mainPanelPinned && mainPanelPinned.length === widgets.length) {


See above for a way to handle the uncertainty of whether the widgets restored will be the same as the widgets whose state was saved.

colin-grant-work · 2021-07-30T19:25:12Z

packages/core/src/browser/shell/application-shell.ts

+    getPinnedBottomWidgets(): boolean[] {
+        const pinned: boolean[] = [];
+
+        toArray(this.bottomPanel.widgets()).forEach((a, i) => {
+            pinned[i] = a.title.className.indexOf('theia-mod-pinned') >= 0;
+        });
+
+        return pinned;
+    }


Not sure that this warrants its own function. Maybe getPinnedWidgetsForArea(area) that can handle both of them?

colin-grant-work · 2021-07-30T19:25:30Z

packages/core/src/browser/shell/application-shell.ts

@@ -643,6 +655,15 @@ export class ApplicationShell extends Widget {
            } else {
                this.collapseBottomPanel();
            }
+            const widgets = toArray(this.bottomPanel.widgets());
+            if (bottomPanel.pinned && bottomPanel.pinned.length === widgets.length) {


Same comment re: handling uncertainty of restoration.

colin-grant-work · 2021-07-30T19:52:01Z

The restoration on sidebars is a little unreliable for me. The following scenario seems to fail consistently.

Using the Electron app (don't know if it matters, but I was)
Open the application and Reset workbench layout
Close application and reopen.
Pin extensions view.
Close application and reopen.
Extensions view is not pinned.

If I move it into the main area and back, then it seems to retain its pinned state even on the sidebar. But if I close and reopen, leave it pinned, close and reopen again, it will have lost its pinned state. Since the only check for pinning is the classname, I'd guess the sidebar is wiping out the classname at some point. On the other hand, an editor on the sidebar seems to retain its pinned state more reliably, so there may be more to it - something related to ViewContainer or maybe AbstractViewContribution? Since the SearchInWorkspace widget shows the same bug, the ViewContribution is maybe a better bet? Or do the sidebars not store their state if they think nothing has happened? When I moved a few widgets out of the left but left the SCM widget there, the SCM widget retained its pinned state, but the Outline view, on the right side, which I hadn't touched, didn't restore its state.

Anyway - tl;dr is that I didn't have any problems in main or bottom areas, but the side panels were a little funky.

dsseng · 2021-07-31T07:53:35Z

Actually, I once noticed view self-unpinning after opening and closing it (switching side panel views). Maybe they are recreated on some conditions?

dsseng · 2021-07-31T07:54:37Z

Btw pinning side panel views is not really used feature, typically they do not need to be protected from being accidentally closed.

colin-grant-work · 2022-02-01T17:32:23Z

@sh7dm, are you interested in further work on this feature?

dsseng · 2022-02-02T06:05:33Z

Not really sure, we should plan required changes

colin-grant-work · 2022-02-08T15:33:01Z

@sh7dm, I think (apart from the conflicts that have arisen in the meantime), the main thing that was holding it back was the side-panel state restoration behavior. I agree with you that that isn't a very common use case, but we should decide whether to just disable it completely in side panels, or we should figure out what's making it buggy. If you're willing to investigate, I'd be happy to take a look at what you find; if you don't have time, I'd be happy to pick up the PR and do some investigation.

dsseng · 2022-02-08T16:59:46Z

Most likely I won't be able to properly debug this as I couldn't do that. Sorry for this problem. You can probably disable it for initial merge and open an issue for that.

dsseng force-pushed the tab-pin branch from e67be1e to c130117 Compare June 20, 2021 08:25

vince-fugnitto reviewed Jun 21, 2021

View reviewed changes

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves shell issues related to the core shell labels Jun 21, 2021

dsseng requested a review from vince-fugnitto June 21, 2021 13:05

vince-fugnitto reviewed Jun 21, 2021

View reviewed changes

packages/core/src/browser/common-frontend-contribution.ts Outdated Show resolved Hide resolved

dsseng requested a review from vince-fugnitto June 21, 2021 17:47

vince-fugnitto reviewed Jun 23, 2021

View reviewed changes

packages/core/src/browser/common-frontend-contribution.ts Show resolved Hide resolved

vince-fugnitto requested a review from colin-grant-work June 23, 2021 13:31

colin-grant-work reviewed Jun 23, 2021

View reviewed changes

packages/core/src/browser/common-frontend-contribution.ts Outdated Show resolved Hide resolved

colin-grant-work reviewed Jun 23, 2021

View reviewed changes

packages/core/src/browser/common-frontend-contribution.ts Outdated Show resolved Hide resolved

packages/core/src/browser/common-frontend-contribution.ts Outdated Show resolved Hide resolved

dsseng requested a review from colin-grant-work June 24, 2021 08:00

Fix lint

e7f625c

dsseng requested a review from vince-fugnitto July 13, 2021 15:52

vince-fugnitto reviewed Jul 19, 2021

View reviewed changes

dsseng added 2 commits July 20, 2021 15:24

shell: save and restore pinned widget state

5198dd9

Signed-off-by: Dmitry Sharshakov <[email protected]>

shell: fix lint

c160f17

Signed-off-by: Dmitry Sharshakov <[email protected]>

dsseng requested a review from vince-fugnitto July 20, 2021 12:58

vince-fugnitto reviewed Jul 20, 2021

View reviewed changes

packages/core/src/browser/shell/application-shell.ts Outdated Show resolved Hide resolved

dsseng added 3 commits July 20, 2021 16:41

shell: separate function for getting pinned main widgets

51d6364

Signed-off-by: Dmitry Sharshakov <[email protected]>

shell: make tabs non-closable when restoring pinned state

8cc1c06

Signed-off-by: Dmitry Sharshakov <[email protected]>

shell: restore bottom panel widgets' pinned state

8023687

Signed-off-by: Dmitry Sharshakov <[email protected]>

dsseng requested a review from vince-fugnitto July 20, 2021 16:06

vince-fugnitto reviewed Jul 26, 2021

View reviewed changes

colin-grant-work reviewed Jul 30, 2021

View reviewed changes

colin-grant-work self-assigned this Feb 1, 2022

colin-grant-work mentioned this pull request Feb 28, 2022

Add Pinned Tabs #10817

Merged

1 task

colin-grant-work closed this in #10817 Mar 8, 2022

Add ability to pin tabs #9614

Add ability to pin tabs #9614

Conversation

dsseng commented Jun 20, 2021 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

dsseng commented Jun 21, 2021 • edited Loading

vince-fugnitto commented Jun 21, 2021

dsseng commented Jun 21, 2021

vince-fugnitto commented Jun 21, 2021

dsseng commented Jun 21, 2021

dsseng commented Jun 21, 2021

vince-fugnitto commented Jun 21, 2021

dsseng commented Jun 21, 2021

vince-fugnitto commented Jun 21, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

dsseng commented Jun 23, 2021

vince-fugnitto commented Jun 23, 2021

dsseng commented Jun 23, 2021 • edited Loading

vince-fugnitto commented Jun 23, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

colin-grant-work commented Jun 23, 2021

dsseng commented Jun 24, 2021

colin-grant-work commented Jun 24, 2021

dsseng commented Jun 24, 2021

colin-grant-work commented Jun 24, 2021 • edited Loading

For rearranging

Against rearranging

vince-fugnitto left a comment • edited Loading

Choose a reason for hiding this comment

dsseng commented Jul 19, 2021

vince-fugnitto commented Jul 19, 2021

colin-grant-work commented Jul 19, 2021 • edited Loading

vince-fugnitto commented Jul 19, 2021

dsseng commented Jul 19, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

dsseng commented Jul 20, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

dsseng commented Jul 26, 2021

colin-grant-work Jul 30, 2021

Choose a reason for hiding this comment

colin-grant-work Jul 30, 2021

Choose a reason for hiding this comment

colin-grant-work Jul 30, 2021

Choose a reason for hiding this comment

colin-grant-work Jul 30, 2021

Choose a reason for hiding this comment

colin-grant-work commented Jul 30, 2021

dsseng commented Jul 31, 2021

dsseng commented Jul 31, 2021 • edited Loading

colin-grant-work commented Feb 1, 2022

dsseng commented Feb 2, 2022

colin-grant-work commented Feb 8, 2022

dsseng commented Feb 8, 2022

dsseng commented Jun 20, 2021 •

edited by vince-fugnitto

Loading

dsseng commented Jun 21, 2021 •

edited

Loading

dsseng commented Jun 23, 2021 •

edited

Loading

colin-grant-work commented Jun 24, 2021 •

edited

Loading

vince-fugnitto left a comment •

edited

Loading

colin-grant-work commented Jul 19, 2021 •

edited

Loading

dsseng commented Jul 31, 2021 •

edited

Loading