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

Introduce Dynamic Tab Resizing Strategy #12360

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

tsmaeder
Copy link
Contributor

What it does

Introduces a new, optional tab layout strategy similar to Chrome tabs as described in #12328
This is part one, introducing only the dynamic tab resizing.

How to test

  1. Turn on the preference "Workbench>Tab>DynamicTabs"
  2. Open and close editors, observing the tab resizing behaviour
  3. Make sure the tabs resizing works correctly when resizing
  4. Make sure other tab usages (like the vertical tabs on the view dock panels) work fine and are unaffected by the dynamic tabs preference
  5. Make sure the tab resizing works correctly when editors have different toolbar actions
  6. Play with the "Workbench>Tab>Minimum Size | Preferred Size" settings and make sure the tab resizing works as expected

Review checklist

Reminder for reviewers

@tsmaeder tsmaeder force-pushed the 12328_dynamic_tabs branch from 197b7c3 to bcb3ef1 Compare March 29, 2023 13:20
@tsmaeder tsmaeder changed the title First cut of dynamic tabs with tab width assignment Introduce Dynamic Tab Resizing Strategy Mar 29, 2023
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Good work! I left some comments in the code.

The new behavior brings in some weirdness: When the tabs fill the whole width and we switch to a new tab which adds/removes decorations, the tab size then only changes once the mouse leaves the tab area. That behavior is as defined but it the sudden jump in tab width feels unintuitive and weird. Maybe we can find a better solution in the long run.
moving-tabs

I found another ugly behavior where the close button is flickering. However I checked, this is already the case on master, so it was not introduced here:
Flickering

packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved
packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved
packages/core/src/browser/core-preferences.ts Outdated Show resolved Hide resolved
packages/core/src/browser/style/tabs.css Outdated Show resolved Hide resolved
@tsmaeder tsmaeder force-pushed the 12328_dynamic_tabs branch from e122b3e to 96040ae Compare April 3, 2023 12:53
@tsmaeder tsmaeder requested a review from sdirix April 11, 2023 09:12
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

Works great for me. I have one small nitpick left (from my own suggestion)

packages/core/src/browser/style/tabs.css Outdated Show resolved Hide resolved
@tsmaeder tsmaeder force-pushed the 12328_dynamic_tabs branch from 96040ae to 477d033 Compare April 11, 2023 15:29
@tsmaeder
Copy link
Contributor Author

@msujew @vince-fugnitto unfortunately, @sdirix is not a committer. Maybe I could get a "Hallelujah" for one of you?

'workbench.tab.shrinkToFit.enabled': {
type: 'boolean',
default: false,
description: nls.localize('theia/core/tabShrinkToFit', 'Shrink tabs to fit available space')
Copy link
Member

Choose a reason for hiding this comment

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

minor: for consistency preferences usually include punctuation

Suggested change
description: nls.localize('theia/core/tabShrinkToFit', 'Shrink tabs to fit available space')
description: nls.localize('theia/core/tabShrinkToFit', 'Shrink tabs to fit available space.')

The same is true for other preferences introduced in the pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other preferences also use whole sentences in the description, where a period at the end makes sense.

@@ -96,13 +96,25 @@ export class DockPanelRenderer implements DockLayout.IRenderer {
@inject(TabBarToolbarRegistry) protected readonly tabBarToolbarRegistry: TabBarToolbarRegistry,
@inject(TabBarToolbarFactory) protected readonly tabBarToolbarFactory: TabBarToolbarFactory,
@inject(BreadcrumbsRendererFactory) protected readonly breadcrumbsRendererFactory: BreadcrumbsRendererFactory,
@inject(CorePreferences) protected readonly corePreferences: CorePreferences
Copy link
Member

Choose a reason for hiding this comment

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

@@ -885,7 +956,7 @@ export class SideTabBar extends ScrollableTabBar {
* Render the tab bar in the _hidden content node_ (see `hiddenContentNode` for explanation),
* then gather size information for labels and render it again in the proper content node.
*/
protected renderTabBar(): void {
protected override updateTabs(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Document as a breaking change?

@@ -571,6 +602,14 @@ export class ScrollableTabBar extends TabBar<Widget> {
if (!this.scrollBar) {
this.scrollBar = this.scrollBarFactory();
}
this.node.addEventListener('mouseenter', () => { this.isMouseOver = true; });
this.node.addEventListener('mouseleave', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This gives a slightly odd behavior if tabs are closed but we remain in the tabbar, perhaps we should recompute when a tab is created/closed?

shrink-to-fit-listener.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is intentional in order to keep the position of the "close" icon stable as we repeatedly close tabs. It's inspired by what Chrome does. I would argue the utility in this case outweighs the visual oddity.

Comment on lines 414 to 421
.p-TabBar-tab .theia-tab-icon-label {
flex: 1;
}

.p-TabBar-tab.p-mod-closable:hover .theia-tab-icon-label,
.p-TabBar-tab.p-mod-current .theia-tab-icon-label {
overflow: hidden;
}
Copy link
Member

Choose a reason for hiding this comment

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

It appears that this styling causes the following regression, and possibly others:

shrink-regression.mov

@vince-fugnitto vince-fugnitto added the ui/ux issues related to user interface / user experience label Apr 12, 2023
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The latest updates look good to me 👍

Part of eclipse-theia#12328

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder tsmaeder force-pushed the 12328_dynamic_tabs branch from 8e0142a to 6fb11a5 Compare April 13, 2023 09:14
@tsmaeder tsmaeder merged commit 4c8f76d into eclipse-theia:master Apr 13, 2023
@tsmaeder tsmaeder mentioned this pull request Apr 13, 2023
1 task
@vince-fugnitto vince-fugnitto added this to the 1.37.0 milestone Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants