Update breadcrumbs when workspace folders update by r3m0t · Pull Request #154616 · microsoft/vscode

r3m0t · 2022-07-09T11:59:10Z

This PR fixes #154615

Test plan at #154615

jrieken

The change overall looks good but it should be done not inn the view but in the model, e.g BreadcrumbsModel. It should listen to the workspace change and trigger its change event

r3m0t · 2022-07-11T21:43:11Z

Thanks @jrieken - I've updated it - can you have a look before I start looking at the tests?

r3m0t · 2022-07-11T21:46:53Z

 	}

+	notifyWorkspaceFoldersChanges(): void {
+		if (this.recomputeOnWorkspaceFolderChange) {


I had a simpler implementation, but there was a bug where the path (dark grey label next to the root folder name) would flash in the explorer view during the manual test.

The issue was explorerViewer.ts is calling setResource instead of setFile, which also means it doesn't pass the option hidePath even though its behaviour is hidePath: true.

As there's so many users of IResourceLabel, I think this change is the safest way, even though the code is a bit messy imo.

I would prefer the "not messy" solution.

how about this way? (new commits on PR)

jrieken

Changing for breadcrumbs looks good. Assigning @bpasero for changes in the labels-world

bpasero

Not liking the changes in labels.ts, I find recomputeOnWorkspaceFolderChange strange. Can we not somehow figure out if an update is needed when a transition happens into workspace? Isn't that like the only time we want to update?

bpasero · 2022-07-12T11:13:30Z


 		this.label = label;
 		this.options = options;
+		this.recomputeOnWorkspaceFolderChange = recomputeOnWorkspaceFolderChange;


This seems strange to me.

r3m0t · 2022-07-12T12:35:26Z

@bpasero the case is that the root folder name changes in the workspace, which can happen by editing the workspace file.

bpasero

Thanks, but I am still not so happy: now we seem to be overwriting the name of a label to be the workspace folder name, but shouldnt that be something only the user of the label set? Isnt the label service typically the driver of a label name?

My worry is that someone sets a name to a label and now gets overwritten by the name of the workspace folder.

What is the actual issue to solve here? Can we not listen in breadcrumbs and update when workspaces change?

r3m0t · 2022-07-13T08:05:02Z

Well, name can be set in three ways - consumer of label service calls setFile and consumer of label service calls setResource. (The third way is setLabel but it isn't relevant)

In this case, "consumer of labels" is "breadcrumbs" and it calls setFile.

The best encapsulation is, the state (ResourceLabelWidget.label.name) is maintained by whoever set it in the first place. So if setFile is used, label service set initial label and should maintain it to the correct value. If setResource is used, consumer of label service set initial label by passing it as the argument IResourceLabelProps.name and it will have to maintain it to the correct value as well by later calls to setResource.

Without this encapsulation, basically setFile is no longer convenient because it doesn't remove the burden on the consumer of label service to be interested in workspace names.

Here's the code from breadcrumbs:

When items change, eg a different tab is selected, don't re-render the common prefix-

vscode/src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts

Line 278 in 764d227

prefix = commonPrefixLength(this._items, items, (a, b) => a.equals(b));

Equality of each breadcrumb for calculating the common prefix is purely by URI -

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

Line 125 in 764d227

return (extUri.isEqual(this.element.uri, other.element.uri) &&

Rendering the new breadcrumbs is by setFile -

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

Line 134 in 764d227

label.element.setFile(this.element.uri, {

So if label service doesn't listen to onDidChangeWorkspaceFolders, then we'll need to change the equals implementation to consider the workspace folder name and change the render implementation to call setResource if FileElement.kind === FileKind.RootFolder so that it can pass the name to the label widget. And, other users of the label service that call setFile could need to put in similar work!

So, let's continue with the label service maintaining the state ResourceLabelWidget.label.name and let consumers continue to use setFile and not look at the workspace folder names.

To prevent accidental updates from the onDidChangeWorkspaceFolders, there's two options, one is to save a flag in setFile. The other is to look at IWorkspaceFoldersChangeEvent? Like, if IWorkspaceFoldersChangeEvent.changed[i].uri == roResource(this.label) this.setFile() . But, the event doesn't give the old folder name so there's no way to check if the label already matches the old root folder name.

I still prefer the original version as it expresses the intent most clearly. Label service set name therefore it should update it. https://github.com/microsoft/vscode/pull/154616/files/4b241555ad2aa48adb695955efda856b397ace75

bpasero · 2022-07-13T11:35:14Z

@r3m0t yeah I get it now, your previous change was nicer in that regard. I updated it a bit, please see: https://gist.github.com/bpasero/53ec674e2d6e0f11237212c87073f994

If you ok with my version and it fixes the issue (I verified it), please add it to the PR. I tried to align things a bit with how we do it in other places in that file.

r3m0t · 2022-07-15T15:31:01Z

Thanks @bpasero , did you mean to check the value of computedWorkspaceFolderLabel in notifyWorkspaceFoldersChanges ? In your current version, it looks like you're only checking the type. It also needs to be added to clear() method. I will get to this in the next few days.

bpasero · 2022-07-15T16:02:11Z

Yeah feel free to further tweak it, thanks!

r3m0t · 2022-07-17T16:40:12Z

@bpasero @jrieken all ready, thanks for your help

vscodenpa assigned jrieken Jul 9, 2022

jrieken suggested changes Jul 11, 2022

View reviewed changes

r3m0t commented Jul 11, 2022

View reviewed changes

jrieken previously approved these changes Jul 12, 2022

View reviewed changes

jrieken requested a review from bpasero July 12, 2022 07:56

bpasero suggested changes Jul 12, 2022

View reviewed changes

bpasero reviewed Jul 12, 2022

View reviewed changes

r3m0t dismissed jrieken’s stale review via dc646a9 July 12, 2022 12:28

bpasero suggested changes Jul 13, 2022

View reviewed changes

bpasero self-assigned this Jul 13, 2022

bpasero added this to the July 2022 milestone Jul 13, 2022

r3m0t force-pushed the breadcrumb-fix branch from 2d452a5 to 1ad4437 Compare July 17, 2022 16:39

Update breadcrumbs when workspace folders update

f605acb

r3m0t force-pushed the breadcrumb-fix branch from 1ad4437 to f605acb Compare July 17, 2022 23:16

bpasero approved these changes Jul 18, 2022

View reviewed changes

bpasero merged commit ed81603 into microsoft:main Jul 18, 2022

jrieken pushed a commit that referenced this pull request Jul 18, 2022

Update breadcrumbs when workspace folders update (#154616)

32c436d

github-actions Bot locked and limited conversation to collaborators Sep 1, 2022

Conversation

r3m0t commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrieken left a comment

Choose a reason for hiding this comment

Uh oh!

r3m0t commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r3m0t Jul 11, 2022 • 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 Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

r3m0t Jul 12, 2022 • 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!

jrieken left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

bpasero Jul 12, 2022

Choose a reason for hiding this comment

Uh oh!

r3m0t commented Jul 12, 2022

Uh oh!

bpasero left a comment

Choose a reason for hiding this comment

Uh oh!

r3m0t commented Jul 13, 2022

Uh oh!

bpasero commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r3m0t commented Jul 15, 2022

Uh oh!

bpasero commented Jul 15, 2022

Uh oh!

r3m0t commented Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

r3m0t commented Jul 9, 2022 •

edited

Loading

r3m0t commented Jul 11, 2022 •

edited

Loading

r3m0t Jul 11, 2022 •

edited

Loading

r3m0t Jul 12, 2022 •

edited

Loading

bpasero commented Jul 13, 2022 •

edited

Loading

r3m0t commented Jul 17, 2022 •

edited

Loading