fix: fix editor progress position when enabled pinnedTabsOnSeparateRow #195314

harbin1053020115 · 2023-10-11T01:39:59Z

To #195313

gjsjohnmurray · 2023-10-11T02:13:12Z

benibenj

Thanks for submitting the bug and creating a PR! I left some comments to your changes

benibenj · 2023-10-11T15:40:54Z

src/vs/workbench/browser/parts/editor/editorTabsControl.ts

@@ -404,12 +404,13 @@ export abstract class EditorTabsControl extends Themable implements IEditorTabsC

 	protected updateTabHeight(): void {
 		this.parent.style.setProperty('--editor-group-tab-height', `${this.tabHeight}px`);
+		this.parent.parentElement?.style.setProperty('--editor-group-tab-total-height', `${this.tabHeight * (this.groupsView.partOptions.pinnedTabsOnSeparateRow ? 2 : 1)}px`);


This probably won't work. Even though the pinnedTabsOnSeparateRow setting might be activated, this doesn't mean that two tab rows are actually visible. If all tabs are pinned or if there is no pinned tab then there will only be one tab row and so the height of the progress bar has to be change in these cases.

We handle the number of tab rows here.

src/vs/workbench/browser/media/part.css

harbin1053020115 · 2023-10-16T03:18:39Z

@benibenj You are right. The first commit won't work. Even pinnedTabsonSeparateRow setting is activated, there will be just one tab row if where is no pinned tabs opened.
And, I found it's not a good way to update tab height dynamic using css variable, we need to conside many scenarios:

When open/close/pinned editors, we need to update height according to opened tabs height
When moving opened editors in editor title, we need to update height according to opened tabs height
When pinnedTabsonSeparateRow/editorTabHeight setting is changed, we need to update height
So, I have changed the way to realize, I moved the editor progress bar into titleContainer, and make the progress bar at the bottom of titleContainer

harbin1053020115 · 2023-10-16T03:26:50Z

Below is the result of progress bar:

normal:
window.density.editorTabHeight setting changed to compact:
breadcrumbs.enabled enabled:
pinnedTabsonSeparateRow enabled:

src/vs/workbench/browser/parts/editor/media/editortitlecontrol.css

benibenj · 2023-10-18T08:38:17Z

I think we should still set the progress bar from the top as we did before. Your approach causes it to be placed underneath the breadcrumbs which did not used to be the case, and so we don't want to change that.

This is my recommendation:

We already know the height of the editor title control. It can be accessed in editorTitleControl.ts. Like in your first approach, you can introduce a CSS variable, and overwrite the absolute top position of the progress bar in editorgroupview.css (This is where the progress bar lives). The variable could be called --editor-group-tabs-height as we already have --editor-group-tab-height. I wouldn't call it title height as we do not want to include the breadcrumbs height.

The last thing to do now is update the value when there are changes to the layout, these could be:

changing window.density.editorTabHeight setting
changing workbench.editor.pinnedTabsOnSeparateRow setting
changing workbench.editor.wrapTabs setting
On layout() (in case of resizing the editor for example) when using wrapped tabs
When opening, closing and moving an editor
When sticking and unsticking (known as pinning and unpinning) an editor

The last two are specific to having some settings turned on, however, we can start off by updating the height every time as it is not an expensive operation and future proof. But we might change this in the future.

harbin1053020115 · 2023-10-19T09:01:51Z

I think we should still set the progress bar from the top as we did before. Your approach causes it to be placed underneath the breadcrumbs which did not used to be the case, and so we don't want to change that.

This is my recommendation:

We already know the height of the editor title control. It can be accessed in editorTitleControl.ts. Like in your first approach, you can introduce a CSS variable, and overwrite the absolute top position of the progress bar in editorgroupview.css (This is where the progress bar lives). The variable could be called --editor-group-tabs-height as we already have --editor-group-tab-height. I wouldn't call it title height as we do not want to include the breadcrumbs height.

The last thing to do now is update the value when there are changes to the layout, these could be:

changing window.density.editorTabHeight setting

changing workbench.editor.pinnedTabsOnSeparateRow setting

changing workbench.editor.wrapTabs setting

On layout() (in case of resizing the editor for example) when using wrapped tabs

When opening, closing and moving an editor

When sticking and unsticking (known as pinning and unpinning) an editor

The last two are specific to having some settings turned on, however, we can start off by updating the height every time as it is not an expensive operation and future proof. But we might change this in the future.

Got it. Let me submit a change.

This reverts commit 302ec88.

This reverts commit ec445b6.

This reverts commit cea64be.

…parateRow" This reverts commit 8f2c0e7.

… position

benibenj · 2023-10-23T09:40:24Z

@harbin1053020115 The change looks great! Thanks very much.
I removed the method you added as we can update the variable in the editorGroupView when we call the layout method.

harbin1053020115 · 2023-10-23T09:50:54Z

@harbin1053020115 The change looks great! Thanks very much. I removed the method you added as we can update the variable in the editorGroupView when we call the layout method.

Great~Moving to layout to reset css variable is a better way.

microsoft#195314) * fix: fix editor progress position when enabled pinnedTabsOnSeparateRow * fix: fix editor progress position * fix: fix editor progress position * fix: add commit to editor title progress style * Revert "fix: add commit to editor title progress style" This reverts commit 302ec88. * Revert "fix: fix editor progress position" This reverts commit ec445b6. * Revert "fix: fix editor progress position" This reverts commit cea64be. * Revert "fix: fix editor progress position when enabled pinnedTabsOnSeparateRow" This reverts commit 8f2c0e7. * feat: add --editor-group-tabs-height css variable to set progress bar position * update height in lyout * Remove setting redraws as already done elsewhere * Remove 2px for progress bit height --------- Co-authored-by: ermin.zem <ermin.zem@alibaba-inc.com> Co-authored-by: BeniBenj <besimmonds@microsoft.com>

fix: fix editor progress position when enabled pinnedTabsOnSeparateRow

8f2c0e7

vscodenpa assigned bpasero Oct 11, 2023

vscodenpa assigned benibenj Oct 11, 2023

bpasero removed their assignment Oct 11, 2023

bpasero added this to the October 2023 milestone Oct 11, 2023

benibenj requested changes Oct 12, 2023

View reviewed changes

alijs added 2 commits October 16, 2023 10:34

fix: fix editor progress position

cea64be

fix: fix editor progress position

ec445b6

harbin1053020115 added 2 commits October 16, 2023 15:48

Merge branch 'main' into fix/editor-progress-position

31a2a8d

Merge branch 'main' into fix/editor-progress-position

8a20911

bpasero reviewed Oct 16, 2023

View reviewed changes

src/vs/workbench/browser/parts/editor/media/editortitlecontrol.css Outdated Show resolved Hide resolved

alijs and others added 3 commits October 17, 2023 10:33

fix: add commit to editor title progress style

302ec88

Merge branch 'main' into fix/editor-progress-position

9da367e

Merge branch 'main' into fix/editor-progress-position

1ef2da7

alijs and others added 7 commits October 23, 2023 15:40

Revert "fix: add commit to editor title progress style"

f7dfb55

This reverts commit 302ec88.

Revert "fix: fix editor progress position"

26ebb78

This reverts commit ec445b6.

Revert "fix: fix editor progress position"

01d8b6e

This reverts commit cea64be.

Revert "fix: fix editor progress position when enabled pinnedTabsOnSe…

e637c21

…parateRow" This reverts commit 8f2c0e7.

feat: add --editor-group-tabs-height css variable to set progress bar…

f66a621

… position

Merge branch 'main' into fix/editor-progress-position

6127f98

update height in lyout

e9bd8cf

benibenj requested a review from bpasero October 23, 2023 09:04

Remove setting redraws as already done elsewhere

c1484c6

Remove 2px for progress bit height

0584d45

bpasero approved these changes Oct 23, 2023

View reviewed changes

benibenj approved these changes Oct 23, 2023

View reviewed changes

bpasero merged commit bf9068c into microsoft:main Oct 23, 2023
5 checks passed

github-actions bot locked and limited conversation to collaborators Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix editor progress position when enabled pinnedTabsOnSeparateRow #195314

fix: fix editor progress position when enabled pinnedTabsOnSeparateRow #195314

harbin1053020115 commented Oct 11, 2023

gjsjohnmurray commented Oct 11, 2023

benibenj left a comment

benibenj Oct 11, 2023

harbin1053020115 commented Oct 16, 2023

harbin1053020115 commented Oct 16, 2023 •

edited

Loading

benibenj commented Oct 18, 2023 •

edited

Loading

harbin1053020115 commented Oct 19, 2023

benibenj commented Oct 23, 2023

harbin1053020115 commented Oct 23, 2023

fix: fix editor progress position when enabled pinnedTabsOnSeparateRow #195314

fix: fix editor progress position when enabled pinnedTabsOnSeparateRow #195314

Conversation

harbin1053020115 commented Oct 11, 2023

gjsjohnmurray commented Oct 11, 2023

benibenj left a comment

Choose a reason for hiding this comment

benibenj Oct 11, 2023

Choose a reason for hiding this comment

harbin1053020115 commented Oct 16, 2023

harbin1053020115 commented Oct 16, 2023 • edited Loading

benibenj commented Oct 18, 2023 • edited Loading

harbin1053020115 commented Oct 19, 2023

benibenj commented Oct 23, 2023

harbin1053020115 commented Oct 23, 2023

harbin1053020115 commented Oct 16, 2023 •

edited

Loading

benibenj commented Oct 18, 2023 •

edited

Loading