Fix for issue 12040 by hun1ahpu · Pull Request #18279 · microsoft/vscode

hun1ahpu · 2017-01-08T19:08:02Z

Fix proposal for issue #12040 : Shortening the paths of the duplicate filenames.

Goal

Path is shortened by replacing excessive info with ellipsis.
Every shortened path has only one match from the original paths. And vice versa for every original path there is only one shortened path that matches it.

Implementation details and assumptions

For every path in the list we search for the shortest subpath that is not a subpath in any other path.
Last segment of the path is treated differently while checking for match as it's next to the filename.
Paths considered as case-sensitive when checking for subpath condition: 'a/b/C' and 'a/b/c' are different. The reason is: in the current implementation the filenames are case-sensitive meaning test.txt and TEST.txt are not duplicates.
Algorithm can be optimized later if needed. Usually the number of duplicate filenames is small.

Future updates (if this PR is accepted):

Always keep disk drive letter at the beginning of the path, same for schema:// (TODOs in unit tests).
Do not replace if path segment is short enough (one letter).

bpasero

@hun1ahpu this is cool, thanks. I gave some feedback in the code.

Also I ran a test quickly and wonder about this situation:

In particular:

first loader.js is inside qunit.1/qunit
second loader.js is inside qunit.1
third loader.js is inside qunit

I find the first and second tab now hard to distinguish, shouldn't the full path be shown in this case?

bpasero · 2017-01-09T09:10:00Z

src/vs/base/common/paths.ts

+ * Every shorten path matches only one original path and vice versa.
+ */
+export function shorten(paths: string[]): string[] {
+	var separator = isWindows ? '\\' : '/';


Suggest to use paths.nativeSep here (https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/paths.ts#L20)

bpasero · 2017-01-09T09:10:27Z

src/vs/base/common/paths.ts

 import { isLinux, isWindows } from 'vs/base/common/platform';
 import { fill } from 'vs/base/common/arrays';
 import { CharCode } from 'vs/base/common/charCode';
+import { endsWith } from 'vs/base/common/strings';


Overall, can you please use const and let instead of var for variables?

bpasero · 2017-01-09T09:11:02Z

src/vs/base/common/paths.ts

+ */
+export function shorten(paths: string[]): string[] {
+	var separator = isWindows ? '\\' : '/';
+	var ellipsis = '...';


How about using the actual unicode ellipsis character? I think that will save us some pixels (e.g. http://www.fileformat.info/info/unicode/char/2026/index.htm)

hun1ahpu · 2017-01-09T22:41:03Z

@bpasero Thank you for your comments. I've resolved comments in the code.
Regarding the other comment I thought the distinction between direct and inderect parent was clear enough. But I agree it is probably confusing. I guess we may consider all the paths like 'a', 'a/...', '.../a' as hard to distinguish. Same for a/b/... and .../a/b ?

bpasero · 2017-01-10T05:44:32Z

src/vs/base/common/paths.ts

 import { fill } from 'vs/base/common/arrays';
 import { rtrim } from 'vs/base/common/strings';
 import { CharCode } from 'vs/base/common/charCode';
+import { endsWith } from 'vs/base/common/strings';


@hun1ahpu unused import?

it is used actually:
https://github.com/hun1ahpu/vscode/blob/896c9850b0394723aab783e39947613fa6597c59/src/vs/base/common/paths.ts#L425

bpasero · 2017-01-10T05:45:04Z

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

 		labelDuplicates.forEach(duplicates => {
 			if (duplicates.length > 1) {
-				duplicates.forEach(duplicate => {
+				var shortenedDescriptions = paths.shorten(duplicates.map(duplicate => duplicate.editor.getDescription()));


var => const

bpasero · 2017-01-10T05:49:24Z

@hun1ahpu here is another example I am not sure about:

We have:

readme.md (top level)
qunit/readme.md => qunit
lib/readme.md => lib
lib/more/readme.md => lib/...
qunit/more/readme.md => qunit/...

To me this does not really improve the way I now understand where the files are from. Imho the right fix here is to show as many segments of the path until the description is unique. So I would expect:

lib/more instead of lib/... and qunit/more instead of qunit/...

…x12040

hun1ahpu · 2017-01-11T19:34:55Z

@bpasero I've resolved your comments. It also works for your example now.z
Thanks.

bpasero · 2017-01-12T06:17:18Z

@hun1ahpu thanks

@Tyriar I am adding you as reviewer to see if the behaviour is now desired. since you filed #12040.

This is for example how terminalService.ts is looking after this change is applied:

Tyriar · 2017-01-12T17:13:48Z

Really cool stuff!

I found one minor problem/preference in that I think it should say . next to files in the root folder:

Tyriar

LGTM from a functionality perspective apart from the minor issue above.

bpasero · 2017-01-12T17:30:35Z

@Tyriar "."? like how? Can you give an example how it would look like?

Tyriar · 2017-01-12T17:44:16Z

@bpasero just a grey . next to gulpfile.js like how it has build. This is a personal preference obviously and would indicate to me, hey you have another gulpfile.js open too (as the tabs may not both be on the screen).

bpasero · 2017-01-12T18:35:30Z

Ah, so the "." would only show if there is a duplicate. Yeah, I dont mind either way.

bpasero · 2017-01-13T14:51:01Z

Merged, thanks for the fix!

bpasero · 2017-01-13T15:36:25Z

@hun1ahpu your tests seem to fail on macOS. Can you fix them? I moved the code into labels.ts and labels.test.ts. Thanks.

hun1ahpu · 2017-01-13T16:48:46Z

@bpasero I will take a look at them tonight.
Also will try to send a couple of updates later: '.' asked by @Tyriar and other improvements.
Thanks.

bpasero · 2017-01-13T18:01:47Z

@hun1ahpu feel free to open a new PR for this, I was a bit too early to merge this in it seems ;)

Fix for issue 12040

3675a25

msftclas added the cla-already-signed label Jan 8, 2017

bpasero self-assigned this Jan 9, 2017

bpasero requested changes Jan 9, 2017

View reviewed changes

Comments resolved

8c0eb56

Merge remote-tracking branch 'upstream/master' into fix12040

896c985

bpasero requested changes Jan 10, 2017

View reviewed changes

hun1ahpu added 3 commits January 10, 2017 08:08

Comments resolved

3bcf216

Remove commented code

ea954ca

Merge branch 'fix12040' of https://github.com/hun1ahpu/vscode into fi…

97cea0d

…x12040

bpasero requested a review from Tyriar January 12, 2017 06:16

Tyriar approved these changes Jan 12, 2017

View reviewed changes

bpasero merged commit 5070ce4 into microsoft:master Jan 13, 2017

bpasero added this to the February 2017 milestone Jan 13, 2017

hun1ahpu deleted the fix12040 branch January 13, 2017 16:46

hun1ahpu mentioned this pull request May 31, 2017

Funny dot rendered in the tab #27718

Closed

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

Conversation

hun1ahpu commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpasero Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

hun1ahpu commented Jan 9, 2017

Uh oh!

bpasero Jan 10, 2017

Choose a reason for hiding this comment

Uh oh!

hun1ahpu Jan 10, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero Jan 10, 2017

Choose a reason for hiding this comment

Uh oh!

bpasero commented Jan 10, 2017

Uh oh!

hun1ahpu commented Jan 11, 2017

Uh oh!

bpasero commented Jan 12, 2017

Uh oh!

Tyriar commented Jan 12, 2017

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero commented Jan 12, 2017

Uh oh!

Tyriar commented Jan 12, 2017

Uh oh!

bpasero commented Jan 12, 2017

Uh oh!

bpasero commented Jan 13, 2017

Uh oh!

bpasero commented Jan 13, 2017

Uh oh!

hun1ahpu commented Jan 13, 2017

Uh oh!

bpasero commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hun1ahpu commented Jan 8, 2017 •

edited

Loading

bpasero left a comment •

edited

Loading