Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add settings for fixed-width tabs #181729

Merged
merged 30 commits into from May 22, 2023
Merged

Conversation

jacekkopecky
Copy link
Contributor

@jacekkopecky jacekkopecky commented May 6, 2023

This PR adds a new setting value for workbench.editor.tabSizing: fixed makes all tabs (except pinned when those are compact or shrunk) the same width; and, like the shrink setting, the tabs can shrink when there are too many.

The PR also adds a setting for the maximum (or default) width: workbench.editor.tabSizingMaxWidth, at least 50px to give enough width for the close button as well as at least the icon or part of the title.

This is meant to address #40290 and is a continuation of the unfinished work from #40750.

This is what it looks like:

2023-05-10 21 33 27

Limitation: the width are fixed solid while the mouse is over the tab bar, so creating a new tab with the keyboard while the mouse is over tabs behaves a wee bit funny.

Let me know what you think.

This is meant at least partially to address microsoft#40290 and
is a continuation of the unfinished work from microsoft#40750.
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice chance of implementing exactly what Google Chrome does for tabs:

  • tabs have a fixed width always and scrollbars never show up
  • when available space is not enough, all tabs shrink evenly
  • when you close a tab, sizes remain until mouse moves out [1]

[1] I think this can easily be achieved with a change such as the one for Atom: https://github.com/atom/tabs/pull/300/files

src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a nice chance of implementing exactly what Google Chrome does for tabs:

  • tabs have a fixed width always and scrollbars never show up
  • when available space is not enough, all tabs shrink evenly
  • when you close a tab, sizes remain until mouse moves out [1]

[1] I think this can easily be achieved with a change such as the one for Atom: https://github.com/atom/tabs/pull/300/files

I think just having a new option to address the linked issue is not sufficient, it would be nice if we can even make this new option useful for people that prefer the browser tab model.

@jacekkopecky
Copy link
Contributor Author

Thanks for the pointer @bpasero , I'll see what I can do.

Tabs shrink uniformly (down to a limit) but stay fixed-width
when the mouse is over the tab bar.
@jacekkopecky
Copy link
Contributor Author

@bpasero done, please let me know what you think. 8-)

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW 👏

Just testing this for 1 minute and it seems exactly what we want.

But give me more time to review closely and see for corner cases.

src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
@bpasero bpasero added this to the Backlog milestone May 11, 2023
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also do some testing of how this new sizing option behaves with:

  • workbench.editor.pinnedTabSizing and pinned tabs in general (pin a tab from its right click menu)
  • workbench.editor.tabCloseButton on left or off
  • workbench.editor.wrapTabs being enabled

src/vs/workbench/browser/workbench.contribution.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/workbench.contribution.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/editor/tabsTitleControl.ts Outdated Show resolved Hide resolved
@jacekkopecky
Copy link
Contributor Author

Thanks for the thorough review @bpasero, I'll address everything, possibly over the weekend.

@jacekkopecky
Copy link
Contributor Author

@bpasero report from testing:

  • workbench.editor.pinnedTabSizing and pinned tabs in general are not affected by this; if pinnedTabSizing is normal, pinned tabs are happily cooperating; if the setting is different, pinned tabs never change size anyway.
  • workbench.editor.tabCloseButton on left or off all behaves well as far as I could see
  • workbench.editor.wrapTabs being enabled introduced cases where the last tab at the end of a line before the wrap would sometimes move to the next line because of size rounding errors, and when I moved over the tab bar quickly after moving out
    • I have handled that by making the last tab in every row a tiny bit smaller when fixed-width so it's guaranteed to fit in its row, and by making the transition more immediate and completing it immediately when we move over the tab bar again so tabs don't get frozen in an intermediate size.

I have one outstanding question on your review, on guidance about disposable listeners; I'll put it in a comment there.

@bpasero
Copy link
Member

bpasero commented May 15, 2023

I still think the transition should only apply when mouse is over the tab stripe, or maybe there is a bug that we think mouse is over when its not? See how here I change the setting value and get an animation:

Recording 2023-05-15 at 08 33 10

@bpasero
Copy link
Member

bpasero commented May 15, 2023

Also, we need to fix tab fading. Here is what is expected when the label is too large (see how s fades):

image

Needs the fix for: #182481

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
@jacekkopecky
Copy link
Contributor Author

I still think the transition should only apply when mouse is over the tab stripe, or maybe there is a bug that we think mouse is over when its not? See how here I change the setting value and get an animation:

I don't understand yet. The way I see it is like this:

  • when the mouse moves onto the tab stripe, nothing should move - the tabs will become fixed-width and if any disappears, the rest will move only to take its place,
  • when the mouse moves out of the stripe, the tabs should snap to the widths that they now should have; it makes sense for that snapping to be a smooth transition rather than instant.

In CSS, this means the transition must be there when the mouse is outside of the tab stripe.

I personally like the transition when the setting changes. Maybe I'm just used to this kind of smooth setting change from somewhere - maybe it's a Mac thing.

@bpasero
Copy link
Member

bpasero commented May 15, 2023

I personally like the transition when the setting changes. Maybe I'm just used to this kind of smooth setting change from somewhere - maybe it's a Mac thing.

Tbh we use transitions very rarely throughout the UI. Mostly to draw attention, for example when showing notifications. We do not use it as a way to convey that something has changed in the UI. I would even argue that for this feature we should maybe not do the transition.

On advice of @bpasero, removed transition because
the editor doesn't really use transition that much.
@bpasero
Copy link
Member

bpasero commented May 15, 2023

I wonder how confusing it is now that the size does not change for other actions that are unrelated to closing:

Recording 2023-05-15 at 22 08 43

@jacekkopecky
Copy link
Contributor Author

I wonder how confusing it is now that the size does not change for other actions that are unrelated to closing:

Yeah, all that complexity is why I suggested simple fixed-width tabs in the first place… 8-)

I have no idea on how confusing it would be; if someone tries it and doesn't like it, they can go back to the previous behaviour and open issues; if that happens, feel free to tag me in case I can address them.

@jacekkopecky
Copy link
Contributor Author

I'll be happy to chat, I'm also on the vscode discord server, but I don't know my way around there much as I've never really used it…

@bpasero
Copy link
Member

bpasero commented May 16, 2023

@jacekkopecky I took the liberty to push more changes, specifically the fixed tabs width now only applies when editors are closed:

https://github.com/microsoft/vscode/pull/181729/files#diff-2cc308dd268844b603ed595ef8cbb6caac75a72a42d51024185932c6df0ac719R578-R582

I will add more feedback in a review.

@bpasero bpasero modified the milestones: Backlog, May 2023 May 16, 2023
@jacekkopecky
Copy link
Contributor Author

@bpasero I'll be busy for a few days, so I may not get to working more on this until Friday, sorry. I'll have a look at your changes and try to record the problem above to better illustrate it.

@bpasero
Copy link
Member

bpasero commented May 22, 2023

@jacekkopecky I was about to approve this but found another case where it can be hard to close multiple tabs with the mouse: some editors show more actions than other editors, so the available space for tabs can grow and shrink while closing. This currently causes the close button to move and thus prevents closing if you keep the mouse over the same spot:

Recording 2023-05-22 at 06 46 39

I think in addition to having tabs be fixed when mouse is over tabs, we also need to prevent actions from drawing and only appear when the mouse moves out?

The code for updating editor toolbar actions is shared between tabs and no-tabs:

protected updateEditorActionsToolbar(): void {
const { primary, secondary } = this.prepareEditorActions(this.getEditorActions());
const editorActionsToolbar = assertIsDefined(this.editorActionsToolbar);
editorActionsToolbar.setActions(prepareActions(primary), prepareActions(secondary));
}

@jacekkopecky
Copy link
Contributor Author

@bpasero in the recording I don't see you closing a tab, but in my testing, I have found this problem (I don't know if it's the one you mean):

When we click the close button, handleClosedEditors fixes the tab widths too late because it's ultimately called by EditorGroupView.doCloseEditor only after the active editor changes, the action icons are updated, and the tab layout changes.

I have pushed code in cac6854 that fixes that problem; I'm not sure if I've done it right, or if we already have a way to register something that should run before the close action.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 , this was a really fun one. You are very fast in responding, that is a pleasure. If you want to pick up any other issue where we ask for help, be my guest 🚀

@bpasero bpasero enabled auto-merge (squash) May 22, 2023 14:57
@bpasero bpasero merged commit cbbd922 into microsoft:main May 22, 2023
6 checks passed
@jacekkopecky
Copy link
Contributor Author

Thank you @bpasero for being patient with me; I'm happy to get this through, mainly for myself. 8-)

@jacekkopecky
Copy link
Contributor Author

jacekkopecky commented May 22, 2023

@bpasero well this is embarrassing, but as you closed #40290 , I looked at it again and saw I missed one corner case – when the tab being closed is the last tab – in which case the tabs might get back to their normal size, as they do in Chrome and Safari.

Here is a fix for your consideration: jacekkopecky@5e85bb6

For illustration, here's a before and after:

2023-05-22 23 05 49

2023-05-22 23 05 18

(my cursor is huge and the recording software misplaces it, but it was right over the close icons)

@bpasero
Copy link
Member

bpasero commented May 23, 2023

@jacekkopecky good catch! want to open a new PR?

@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants