Add border for dirty tabs #59759

usernamehw · 2018-10-01T17:48:52Z

Adds a themable top border for tabs.
Disabled in hc themes.
Disabled if tab.activeBorderTop is set.

2px border does't look very good using light themes(especially fullscreen).

bpasero · 2018-10-02T05:01:51Z

@usernamehw I briefly looked into this last milestone and I think to do this right, the following is needed:

have a setting to enable/disable this, not everyone likes to see dirty tabs with a border so it can only be optional and disabled by default
a tab can be in 4 states (active focused, active unfocused, inactive focused, inactive unfocused) so this color needs to be expanded to 4 colors I think
instead of creating a CSS rule I would much rather like to see this added to redrawTab as style on the element directly
I find 3px border quite a lot, I think 2px is fine too

Thanks

1. Make it thinner 2. Add a setting to disable it 3. Use 4 colors (activeFocused, activeUnfocused, inactiveFocused, inactiveUnfocused) 4. Move part of logic

usernamehw · 2018-10-03T20:27:43Z

cc @bpasero

bpasero

I left more feedback. Also: given this is now a setting, all our built in themes should provide a color for this so that a user that enables this setting gets a nice experience for our themes. The theme defaults are all defined in our extensions folder:

bpasero · 2018-10-04T04:47:41Z

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

@@ -30,6 +30,7 @@ export interface IEditorPartOptions extends IWorkbenchEditorPartConfiguration {

 export const DEFAULT_EDITOR_PART_OPTIONS: IEditorPartOptions = {
 	showTabs: true,
+	dirtyTabBorder: false,


Suggest: highlightModifiedTabs

bpasero · 2018-10-04T04:47:55Z

src/vs/workbench/common/editor.ts

@@ -938,6 +938,7 @@ export interface IWorkbenchEditorConfiguration {

 export interface IWorkbenchEditorPartConfiguration {
 	showTabs?: boolean;
+	dirtyTabBorder?: boolean;


Suggest: highlightModifiedTabs

bpasero · 2018-10-04T04:48:55Z

src/vs/workbench/common/theme.ts

@@ -66,6 +66,30 @@ export const TAB_ACTIVE_BORDER_TOP = registerColor('tab.activeBorderTop', {
 	hc: null
 }, nls.localize('tabActiveBorderTop', "Border to the top of an active tab. Tabs are the containers for editors in the editor area. Multiple tabs can be opened in one editor group. There can be multiple editor groups."));

+export const TAB_DIRTY_ACTIVE_FOCUSED_BORDER = registerColor('tab.dirtyActiveFocusedBorder', {


To make it easier for theme authors, 3 of the 4 colors should be derived from the one main color. E.g. you can reference the main color and then apply opacity to the color to alter it. That way, a theme can only define the main color and is all set.

bpasero · 2018-10-04T04:49:23Z

src/vs/workbench/common/theme.ts

+	hc: null
+}, nls.localize('tabDirtyActiveFocusedBorder', "Border on the top of dirty active tab in active editor group."));
+
+export const TAB_DIRTY_ACTIVE_UNFOCUSED_BORDER = registerColor('tab.dirtyActiveUnfocusedBorder', {


I suggest tab.modified instead of tab.dirty for the colors

bpasero · 2018-10-04T04:49:30Z

src/vs/workbench/electron-browser/main.contribution.ts

@@ -480,6 +480,11 @@ configurationRegistry.registerConfiguration({
 			'description': nls.localize('showEditorTabs', "Controls whether opened editors should show in tabs or not."),
 			'default': true
 		},
+		'workbench.editor.dirtyTabBorder': {


Suggest: highlightModifiedTabs

bpasero · 2018-10-04T04:50:03Z

src/vs/workbench/electron-browser/main.contribution.ts

@@ -480,6 +480,11 @@ configurationRegistry.registerConfiguration({
 			'description': nls.localize('showEditorTabs', "Controls whether opened editors should show in tabs or not."),
 			'default': true
 		},
+		'workbench.editor.dirtyTabBorder': {
+			'type': 'boolean',
+			'description': nls.localize('showEditorDirtyTabBorder', "Controls whether top border is drawn on dirty file tabs or not."),


"Controls wether modified tabs are highlighted"

I think some people would search for border keyword to look it up in settings. It already has highlight in setting itself; combined with border should provide more searchable result.

bpasero · 2018-10-04T04:50:58Z

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

@@ -908,8 +909,15 @@ export class TabsTitleControl extends TitleControl {
 	private redrawEditorDirty(editor: IEditorInput, tabContainer: HTMLElement): void {
 		if (editor.isDirty()) {
 			addClass(tabContainer, 'dirty');
+
+			if (this.accessor.partOptions.dirtyTabBorder && !this.getColor(TAB_ACTIVE_BORDER_TOP)) {


Sorry for not being clear here: instead of assigning a CSS class, why not set the border color directly on the element? I wanted to avoid having a new CSS rule produced for this feature.

bpasero · 2018-10-04T04:53:11Z

src/vs/workbench/common/theme.ts

@@ -66,6 +66,30 @@ export const TAB_ACTIVE_BORDER_TOP = registerColor('tab.activeBorderTop', {
 	hc: null
 }, nls.localize('tabActiveBorderTop', "Border to the top of an active tab. Tabs are the containers for editors in the editor area. Multiple tabs can be opened in one editor group. There can be multiple editor groups."));

+export const TAB_DIRTY_ACTIVE_FOCUSED_BORDER = registerColor('tab.dirtyActiveFocusedBorder', {
+	dark: null,


I suggest to assign a default color here for dark and light theme so that enabling the setting has a visible impact.

usernamehw · 2018-10-05T15:39:26Z

@bpasero , can't make it work with box-shadow because it conflicts too much. Is there any important reason it's not using borders in the first place?

In any way, awaiting more feedback.

bpasero · 2018-10-05T16:26:06Z

@usernamehw actually that is a good point, why are you using box-shadow for this? Can you not use border in the same way this is done for the existing top border?

usernamehw · 2018-10-05T16:26:53Z

Using 1px height div?

usernamehw · 2018-10-05T16:27:46Z

It's almost working now, I need only to fix 2px shifted tab.activeBorderTop...

bpasero · 2018-10-05T16:32:51Z

@usernamehw yeah maybe you can use the same element we already use for the border and make it 2px instead.

bpasero · 2018-10-06T06:42:41Z

@usernamehw I was not suggesting to set a border on each tab (this will have the ugly side effect of pushing down the label within) but rather reuse the DIV that is the container of the top border already (https://github.com/Microsoft/vscode/blob/c09511ccc1f67b08ebda9141d49a030cc381865e/src/vs/workbench/browser/parts/editor/tabsTitleControl.ts#L881). Can we not utilize that one for this purpose since it is already there? Should make things much simpler.

usernamehw · 2018-10-06T09:40:52Z

@bpasero, This should be close to the final version.

I commented out 2 lines that seemed redundant... Is there a reason they were there?:

tabContainer.style.removeProperty('--tab-border-bottom-color');
// ...
tabContainer.style.removeProperty('--tab-border-top-color');

usernamehw · 2018-10-06T10:15:03Z

Should workbench.editor.highlightModifiedTabs be added to telemetry?

https://github.com/Microsoft/vscode/blob/c09511ccc1f67b08ebda9141d49a030cc381865e/src/vs/platform/telemetry/common/telemetryUtils.ts#L87-L92

bpasero · 2018-10-07T07:26:46Z

@usernamehw

I commented out 2 lines that seemed redundant... Is there a reason they were there?

I think its fine to leave them, we remove a CSS variable that we added before. Its more a cleanup operation without visual impact.

Should workbench.editor.highlightModifiedTabs be added to telemetry?

Yes, why not. This list is probably out of date by now anyways, I typically forget to update it.

bpasero · 2018-10-08T08:26:57Z

Thanks, this is good to merge. I did some slight changes:

I think its fine to allow for both modified border color and top border color if the theme sets this. Anything else would probably make the user thing something is broken. The modified color will not win though if the active tab has a border top color specified.
I tweaked the defaults of the colors to apply the same dimming as for the foreground color of a tab to be in sync, let me know if that does not work for you

bpasero · 2018-10-08T08:46:43Z

Actually I will push another change to ensure that the dirty border always wins even if a normal border is configured. It seems to me the modified border is much more specific compared to a normal border from the theme and should always win.

Theming for dirty tabs

072dfb4

RMacfarlane assigned bpasero Oct 1, 2018

bpasero added this to the On Deck milestone Oct 2, 2018

usernamehw added 2 commits October 3, 2018 23:00

Improve border for dirty tabs

60be08d

1. Make it thinner 2. Add a setting to disable it 3. Use 4 colors (activeFocused, activeUnfocused, inactiveFocused, inactiveUnfocused) 4. Move part of logic

Hot apply of setting dirtyTabBorder

c8bd0cd

bpasero requested changes Oct 4, 2018

View reviewed changes

usernamehw added 9 commits October 4, 2018 19:38

Rename the setting

b34e8d5

Add default dirty border for all themes

7a2456c

3 of the 4 colors should be derived from the one main color

b022a2a

Rename dirty to modified

6d87b50

Rename modifiedActiveFocusedBorder to modifiedBorder

8d0aead

Add modified border color for built-in themes

fd95472

More contrast

ca6a691

Applying style directly on element

1563dd3

Changing only color

3e0953c

Using full border

7642f45

Using existing div for border styling

57e1ae6

usernamehw added 2 commits October 7, 2018 13:34

Add setting to telemetry

1c2324a

Uncomment cleanup code

fd8f5d5

bpasero added 2 commits October 8, 2018 09:45

tweak colors and descriptions

630b0b3

💄

5890ba7

bpasero modified the milestones: On Deck, October 2018 Oct 8, 2018

bpasero merged commit e891e2f into microsoft:master Oct 8, 2018

usernamehw deleted the dirty branch October 8, 2018 09:35

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Add border for dirty tabs #59759

Add border for dirty tabs #59759

Uh oh!

Conversation

usernamehw commented Oct 1, 2018

Uh oh!

bpasero commented Oct 2, 2018

Uh oh!

usernamehw commented Oct 3, 2018

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usernamehw commented Oct 5, 2018

Uh oh!

bpasero commented Oct 5, 2018

Uh oh!

usernamehw commented Oct 5, 2018

Uh oh!

usernamehw commented Oct 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero commented Oct 5, 2018

Uh oh!

bpasero commented Oct 6, 2018

Uh oh!

usernamehw commented Oct 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usernamehw commented Oct 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero commented Oct 7, 2018

Uh oh!

bpasero commented Oct 8, 2018

Uh oh!

bpasero commented Oct 8, 2018

Uh oh!

Uh oh!

usernamehw commented Oct 5, 2018 •

edited

Loading

usernamehw commented Oct 6, 2018 •

edited

Loading

usernamehw commented Oct 6, 2018 •

edited

Loading