Add settings to control the tab subtitle style (#12965) #33815

tvald · 2017-09-04T20:36:18Z

Adds a template system for tab descriptions:

${pathShort}: e.g. parent
${pathMedium}: e.g. more/parent
${pathLong}: e.g. workspace/even/more/parent
${pathDifferential}: e.g. .../parent, when another tab shares the same title (current behavior)

msftclas · 2017-09-04T20:36:22Z

@tvald,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

tvald · 2017-09-04T22:32:32Z

To anticipate a couple of design questions:

Why use a template instead of a simple enumeration?

The template system is more flexible and future-proof than an enumeration. The comparable configuration window.title also uses a template system.

Is ${pathMedium} useful?

Probably not. My initial impulse was to provide ancestor directories via ${path[0..N]}, where ${path[0]} = 'parent' all the way up to ${path[N]} = 'workspace'. However, this would abuse the template system with a faux-array syntax. I settled on pathMedium as an inoffensive partial solution for anyone who wants something in-between pathShort and pathLong.

I would prefer to see a minimal feature accepted and merged. It can then be expanded with further PRs as users figure out what they more want in the tab description.

bpasero

Good start, some feedback provided.

bpasero · 2017-09-06T05:01:27Z

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

@@ -104,6 +104,16 @@ let workbenchProperties: { [path: string]: IJSONSchema; } = {
 		'description': nls.localize('showEditorTabs', "Controls if opened editors should show in tabs or not."),
 		'default': true
 	},
+	'workbench.editor.tabDescription': {


I wonder how clear this setting for users is. I know we call this "description" internally, but unsure if people will relate this to the small text next to the main label of a tab.

We already have window.title, maybe this should be workbench.editor.tabSubtitle ?

bpasero · 2017-09-06T05:09:52Z

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

@@ -211,6 +214,10 @@ export class TabsTitleControl extends TitleControl {
 			}
 		}));

+		// Configuration updates


Can we try to put this into the ITabOptions bag and use the related IEditorGroupsService.onTabOptionsChanged event?

I looked into this briefly, and I don't know how to wire it all up. Sorry.

ok leave it as is and I will have a look after you are done 👍

bpasero · 2017-09-06T05:10:57Z

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

@@ -211,6 +214,10 @@ export class TabsTitleControl extends TitleControl {
 			}
 		}));

+		// Configuration updates
+		this.toUnbind.push(this.configurationService.onDidUpdateConfiguration(() => this.onConfigurationChanged()));


I typically prefer to setup listeners from the constructor via registerListeners (see in parent class: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/titleControl.ts#L146). Can we do the same in this class and call the super one?

I attempted this like so:

protected registerListeners(): void { super.registerListeners(); // Configuration updates this.toUnbind.push(this.configurationService.onDidUpdateConfiguration(() => this.onConfigurationChanged())); this.onConfigurationChanged(false); }

Which results in the following error on startup:

Cannot read property 'onDidUpdateConfiguration' of undefined at TabsTitleControl.registerListeners (file:///Users/tvald/Documents/Tuple/git/vscode/out/vs/workbench/browser/parts/editor/tabsTitleControl.js:47:57)

It appears that configurationService isn't yet initialized when registerListeners() is called from super(). I haven't looked into the injection framework, so I don't know why configurationService is different from everything else being injected.

ok leave it as is and I will have a look after you are done 👍

bpasero · 2017-09-06T05:11:29Z

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

@@ -211,6 +214,10 @@ export class TabsTitleControl extends TitleControl {
 			}
 		}));

+		// Configuration updates
+		this.toUnbind.push(this.configurationService.onDidUpdateConfiguration(() => this.onConfigurationChanged()));
+		this.onConfigurationChanged();


This call seems to cause an update even though we are in the middle of creating the control. I would add an extra flag to not cause an update if this method is being called from construction phase.

fixed in d925602

bpasero · 2017-09-06T05:14:17Z

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

 					});
 				}
 			}
 		});

+		for (const label of labels) {


This seems to be a very specific way of treating description labels by using the native path separator. You have to keep in mind though that there are editors that are not about files (e.g. extension editor).

I think a better approach is to push the logic of a short, medium or long description to each EditorInput. If you see how it is done for the window title (FileEditorInput.getTitle()) we should do something similar for the description imho. We can just change the getDescription signature to accept short, medium or long version (we already have a verbose flag which is going into that direction).

That does sound better, but it's more involved than I can commit to.

As I mentioned before, just move the logic into FileEditorInput, no need to implement it for all other editors.

bpasero · 2017-09-06T05:16:00Z

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

+		'default': '${pathDifferential}',
+		'description': nls.localize({ comment: ['This is the description for a setting. Values surrounded by parenthesis are not to be translated.'], key: 'tabDescription' },
+			`Controls tab descriptions. Variables are substituted based on the context:
+\${pathShort}: e.g. parent


This seems to be a very specific way of treating description labels by using the native path separator. You have to keep in mind though that there are editors that are not about files (e.g. extension editor). I would rather call this:

descriptionShort

descriptionMedium

descriptionLong

descriptionDifferential

Do note that descriptions are already assumed to be paths when they're trimmed via shorten. That said, the architecture you mention below sounds sufficiently flexible and could be augmented to provide an editor-type-specific shorten method as well. Unfortunately, that's more involved than I can commit to.

Yes, I agree that the shorten logic is assuming paths but that is only because this algorithm can only work with paths and nothing else, so I think that is fair. But other editors might have the desire to provide short, medium and long descriptions.

I think the change is straightforward: I am not asking to implement this for each and every editor, it is fine to just move your logic into FileEditorInput.

…g initialization.

- Change configuration type to the enum ['default', 'short', 'medium', 'long'] - Move path logic into FileEditorInput. - Move configuration handling into GroupService/ITabOptions. - Refactor duplicate path shortening logic.

tvald · 2017-09-07T10:48:24Z

The requested changes are done in d8b5863. This required a couple of significant changes:

the configuration value is now just an enum 'default' | 'short' | 'medium' | 'long'
long uses the absolute path, while medium uses a path relative to the workspace root

bpasero · 2017-09-07T15:59:41Z

@tvald thanks, I will have a look tomorrow.

… show subtitle when all descriptions are identical.

tvald · 2017-09-08T19:13:03Z

Added a couple of small fixes to handle corner cases in the de-duplication logic.

bpasero

@tvald this took me quite some time to review, I left some comments.

Can you elaborate why you decided to rewrite the code within the if (shortenDuplicates) { branch from the version I had before? It seems to me that you could have left that code intact and just use getDescription(LONG) instead of getDescription(true) for picking the full description when there are duplicates.

bpasero · 2017-09-09T06:16:32Z

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

+		'enum': ['default', 'short', 'medium', 'long'],
+		'default': 'default',
+		'description': nls.localize({ comment: ['This is the description for a setting. Values surrounded by parenthesis are not to be translated.'], key: 'tabDescription' },
+			`Controls tab subtitle format:


Maybe a bit more verbose to understand the setting: Controls the format of the subtitle for an editor tab. Subtitles show up next to the file name depending on this setting and make it easier to understand the location of a file:

done in 26db623

bpasero · 2017-09-09T06:17:59Z

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

+short:   'parent'
+medium:  'workspace/src/parent'
+long:    '/home/user/workspace/src/parent'
+default: '.../parent', when another tab shares the same title`),


Prefix the enumeration values with "-" to be consistent with other settings where we do that, e.g. workbench.fontAliasing setting below.

done in 26db623

bpasero · 2017-09-09T06:18:45Z

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

@@ -104,6 +104,17 @@ let workbenchProperties: { [path: string]: IJSONSchema; } = {
 		'description': nls.localize('showEditorTabs', "Controls if opened editors should show in tabs or not."),
 		'default': true
 	},
+	'workbench.editor.tabSubtitle': {


Please also define the enumDescriptions property to get more help when typing. See https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/electron-browser/main.contribution.ts#L229 for example how we use it for the workbench.fontAliasing setting.

done in 26db623

bpasero · 2017-09-09T06:28:54Z

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

@@ -76,7 +75,7 @@ export class TabsTitleControl extends TitleControl {
 		@IWindowsService private windowsService: IWindowsService,
 		@IThemeService themeService: IThemeService,
 		@IFileService private fileService: IFileService,
-		@IWorkspacesService private workspacesService: IWorkspacesService
+		@IWorkspacesService private workspacesService: IWorkspacesService,


Looks like an extra trailing comma.

done in 26db623

bpasero · 2017-09-09T06:45:19Z

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

+			// identify duplicate titles
+			const mapTitleToDuplicates = new Map<string, AugmentedLabel[]>();
+			for (const label of labels) {
+				getOrSet(mapTitleToDuplicates, label.name, []).push(label);


I think we can already here ignore any IEditorLabel that does not have a description. Because further below there is a check:

if (typeof label.description !== 'string' || !label.description) { label.description = ''; }

and then later:

duplicateTitles = duplicateTitles.filter(label => label.description);

If we would just ignore labels without description in the code path of handling duplicates I think we can simplify this a bit.

done in 26db623

bpasero · 2017-09-09T06:57:57Z

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

+				const shortenedDescriptions = shorten(descriptions);
+				descriptions.forEach((description, i) => {
+					const duplicateDescriptions = mapDescriptionToDuplicates.get(description);
+					duplicateDescriptions.map(label => label.description = shortenedDescriptions[i]);


Use forEach() here and not map()?

done in 26db623

tvald · 2017-09-10T07:32:22Z

Can you elaborate why you decided to rewrite the code within the if (shortenDuplicates) { branch from the version I had before?

There's probably a clean way to retain the old code, if that's desired. Some small rearrangement was necessary to switch from the template system to the enum configuration, and at the time it seemed cleaner to rewrite the whole section. I'm not attached to the new code.

bpasero · 2017-09-11T10:34:29Z

@tvald thanks, I will keep your changes in around handling duplicates.

Add template system for tab descriptions. (microsoft#12965)

3de0303

msftclas added the cla-signed label Sep 4, 2017

This was referenced Sep 4, 2017

Add configuration to always show tab descriptions. (#12965) #33718

Closed

Provide an option to always show parent folder for files in tabs #12965

Closed

bpasero added this to the On Deck milestone Sep 5, 2017

bpasero self-assigned this Sep 5, 2017

bpasero requested changes Sep 6, 2017

View reviewed changes

tvald added 2 commits September 5, 2017 23:53

Add a boolean flag to onConfigurationChanged to prevent updates durin…

d925602

…g initialization.

- Change configuration key to 'workbench.editor.tabSubtitle'.

d8b5863

- Change configuration type to the enum ['default', 'short', 'medium', 'long'] - Move path logic into FileEditorInput. - Move configuration handling into GroupService/ITabOptions. - Refactor duplicate path shortening logic.

Don't consider empty-string descriptions during de-duplication. Don't…

6532816

… show subtitle when all descriptions are identical.

bpasero requested changes Sep 9, 2017

View reviewed changes

Minor fixes and documentation.

26db623

bpasero merged commit 3d6f59a into microsoft:master Sep 11, 2017

bpasero modified the milestones: September 2017, On Deck Sep 11, 2017

bpasero changed the title ~~Add template system for tab descriptions. (#12965)~~ Add settings to control the tab subtitle style (#12965) Sep 11, 2017

bpasero mentioned this pull request Sep 18, 2017

Tab title uses parent for specified file names #30316

Closed

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

Add settings to control the tab subtitle style (#12965) #33815

Add settings to control the tab subtitle style (#12965) #33815

Conversation

tvald commented Sep 4, 2017 • edited by bpasero Loading

msftclas commented Sep 4, 2017

tvald commented Sep 4, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvald Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvald commented Sep 7, 2017

bpasero commented Sep 7, 2017

tvald commented Sep 8, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvald commented Sep 10, 2017

bpasero commented Sep 11, 2017

tvald commented Sep 4, 2017 •

edited by bpasero

Loading

tvald Sep 6, 2017 •

edited

Loading