Make symbol link icon to be refreshed correctly #168707

ShuiRuTian · 2022-12-10T09:03:40Z

Another try to fix #167146 by reference #167828 (comment)

This PR makes the following changes:

IDecorationsProvider is able to fire another event onForceRefresh, to tell which URI need to be refreshed.
DecorationsService will listen to the event, record the URIs that need to be refreshed. And when getting values, it will get the fresh value rather than cache from the IDecorationsProvider if the IDecorationsProvider and URI are both same.

However, this PR is kind of noisy, because it adds onForceRefresh to all IDecorationsProviders. Should this method be optional?

ShuiRuTian · 2022-12-10T09:04:34Z

Ping @jrieken and @lramos15

lramos15 · 2022-12-12T16:15:28Z

Looks like some unit tests are failing here.

jrieken · 2022-12-12T16:45:06Z

@ShuiRuTian I really don't understand why this needs any changes in the decoration service and why this isn't a problem of the concrete provider, e.g of ExplorerDecorationsProvider. It has the ability to signal changes for one or many resources and the decoration service will re-request data from it in return. So, please explain why these complex changes are needed. Thanks

ShuiRuTian · 2022-12-13T08:17:32Z

Hi, @jrieken

The main purpose is to be a bit more accurate and effective.

As we could see, ExplorerDecorationsProvider does not listen to file change event:

So what about listening to FileService.onDidFilesChange?
It could work, but the data need to be refetched each time when the resource is updated. Because we have no idea whether it was a symbolic link or not last time.(The data is stored in templateData.label, in which we decide whether the decorations value need to be updated or not.)

To avoid this, this PR add another Emitter to provider, which is very similar to the existing Event. The purpose is to give more control to the client(which use the service):

Here, it's able to decide whether the value need to be refreshed or not.

However, I agree it's kind of complex, maybe it's better to just fix the issue with listen to fileService.onDidFilesChange at first, and if it's proven this is an issue in the future, we could fix it. Is this preferred?

jrieken · 2022-12-13T16:19:48Z

So what about listening to FileService.onDidFilesChange?

I leave it up to @lramos15 to know but generally the place to fix this should be the one provider that's wrong, not the service around them

lramos15 · 2022-12-13T16:47:20Z

@ShuiRuTian Are there worries about listening to it in the decoration provider? Is this a performance question? I would definitely attempt that solution first since this is explorer specific and then run a performance profile of some sort. Thanks for looking into this!

ShuiRuTian · 2022-12-14T09:53:42Z

Thanks for the great suggestions, code is updated.
Ping @jrieken and @lramos15

lramos15 · 2022-12-14T17:26:55Z

FYI @bpasero Any concerns about the update to the label file or listening to the file service like this?

bpasero · 2022-12-15T06:32:45Z

src/vs/workbench/browser/labels.ts

 		});
 	}

+	private hasIsSymbolicLinkChanged(newOptions?: IResourceLabelOptions): boolean {


hasSymbolicLinkChanged?

bpasero · 2022-12-15T06:32:52Z

src/vs/workbench/browser/labels.ts

@@ -456,6 +461,7 @@ class ResourceLabelWidget extends IconLabel {
 			}
 		}

+		const hasIsSymbolicLinkChanged = this.hasIsSymbolicLinkChanged(options);


hasSymbolicLinkChanged?

bpasero · 2022-12-15T06:36:39Z

src/vs/workbench/contrib/files/browser/views/explorerDecorationsProvider.ts

@@ -59,6 +61,9 @@ export class ExplorerDecorationsProvider implements IDecorationsProvider {
 		this.toDispose.add(explorerRootErrorEmitter.event((resource => {
 			this._onDidChange.fire([resource]);
 		})));
+		this.toDispose.add(fileService.onDidFilesChange(e => {


This event can fire thousands of times at the same time for thousands of files, so whoever is listening needs to be checked for perf bottlenecks if that happens.

bpasero · 2022-12-15T06:38:00Z

We should at least measure the perf baseline on a slower machine (maybe also OS = Windows) when thousands of events come in, e.g. from npm install. Listening to file events and re-emitting that as a change you always have to be fit for potential 10-thousands of events firing at the same time.

I am not 100% sure the (relatively minor) issue deserves such a fix when it makes perf worse tbh.

ShuiRuTian · 2022-12-16T04:17:23Z

So we do not want to introduce any potential performance issue or any complexity? Then maybe we could close this PR to keep the house clean.

bpasero · 2022-12-16T06:05:26Z

So we do not want to introduce any potential performance issue or any complexity?

We do not want to introduce performance issues. File events are a potential source of performance issue because the amount of events can be very very high. We unfortunately not have a way to listen for specific events, but in reality we would probably only be interested in events for resources that are visible in the explorer.

ShuiRuTian · 2022-12-16T09:40:28Z

At the beginning, this PR adds another Emitter to each decoration provider, so that we could give more control to the caller and avoid the performance issue because we only fetch data when symbolic link is changed. You could reference this comment.

But I agree it's indeed kind of weird. And the reviewers give different opinions here and here.

I have no idea, let the team decide it...

PS: I used force push, so the history is gone...

lramos15 · 2022-12-16T17:43:09Z

@ShuiRuTian I wonder, could we listen to the explorer refresh and then iterating through just the files the explorer knows of checking if they're symbolic links? Like Ben said this is expensive for an edge case and I would like to resolve it, but not at the cost of perf. If you don't have time to investigate such a complex issue I understand and can take a look myself when I have some free cycles.

ShuiRuTian · 2022-12-19T15:13:18Z

@lramos15 Sorry for the late response.

Sounds great, but how do we know whether one explorer item was symbolic link or not? I think this info is kind of like an implementation detail now and it's hidden in high level. Or we need to reactor a bit to get this message?

lramos15 · 2023-02-27T21:25:55Z

@lramos15 Sorry for the late response.

Sounds great, but how do we know whether one explorer item was symbolic link or not? I think this info is kind of like an implementation detail now and it's hidden in high level. Or we need to reactor a bit to get this message?

You would likely need to do some refactoring to get the message to the right place

evur · 2023-04-20T16:51:44Z

When will this be finished?

vivodi · 2024-12-29T16:56:19Z

Is this still relevant?

ShuiRuTian changed the title ~~Another try to fix 167146~~ Make symbol link icon to be refreshed correctly Dec 10, 2022

vscodenpa assigned lramos15 Dec 10, 2022

ShuiRuTian force-pushed the another-try-to-fix-167146 branch from 627d7c7 to 82b2548 Compare December 14, 2022 08:58

The symbol link icon could be refreshed when file is changed

7731fd4

ShuiRuTian force-pushed the another-try-to-fix-167146 branch from 82b2548 to 7731fd4 Compare December 14, 2022 09:50

bpasero reviewed Dec 15, 2022

View reviewed changes

Make symbol link icon to be refreshed correctly #168707

Are you sure you want to change the base?

Make symbol link icon to be refreshed correctly #168707

Conversation

ShuiRuTian commented Dec 10, 2022

Uh oh!

ShuiRuTian commented Dec 10, 2022

Uh oh!

lramos15 commented Dec 12, 2022

Uh oh!

jrieken commented Dec 12, 2022

Uh oh!

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

Uh oh!

Uh oh!

jrieken commented Dec 13, 2022

Uh oh!

lramos15 commented Dec 13, 2022

Uh oh!

ShuiRuTian commented Dec 14, 2022

Uh oh!

lramos15 commented Dec 14, 2022

Uh oh!

bpasero Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

bpasero Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

bpasero Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ShuiRuTian commented Dec 16, 2022

Uh oh!

bpasero commented Dec 16, 2022

Uh oh!

ShuiRuTian commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lramos15 commented Dec 16, 2022

Uh oh!

ShuiRuTian commented Dec 19, 2022

Uh oh!

lramos15 commented Feb 27, 2023

Uh oh!

evur commented Apr 20, 2023

Uh oh!

vivodi commented Dec 29, 2024

Uh oh!

Uh oh!

ShuiRuTian commented Dec 13, 2022 •

edited

Loading

bpasero commented Dec 15, 2022 •

edited

Loading

ShuiRuTian commented Dec 16, 2022 •

edited

Loading