Allow label providers to notify that changes require a refresh #5884

westbury · 2019-08-07T20:37:58Z

Currently label providers can provide names, descriptions, and icons. However the result is cached so once the provider has provided a property, the tree cannot be refreshed with a changed value.

We need to be able to refresh the navigation tree with changed values because we have an icon we set to a directory, but the icon depends on other files next to the directory. These other files may change and this requires updating the icon for the directory. Note that the Eclipse label providers provide a listener so that label providers can notify the UI components that they must refresh (see https://help.eclipse.org/neon/topic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/jface/viewers/IBaseLabelProvider.html#addListener-org.eclipse.jface.viewers.ILabelProviderListener- ). The navigation tree already has an emitter to force a refresh so it is not that much more code to connect this to the label providers.

How to test

There is no easy way to test this. I have tested it in our application where I can rename a file back and forth and see the icon for a folder change back and forth.

To test it, one would have to modify a label provider to provide a name or icon that depends on something other than the uri. Perhaps it could depend on the existence of another file. Then add a file watcher to the label provider and fire the event. Here are some bits of code that may help:

import { FileChangeType, FileSystemWatcher } from '@theia/filesystem/lib/browser';

and

    protected readonly onElementUpdatedEmitter = new Emitter<URI>();

and

        @inject(FileSystem) protected fileSystem: FileSystem;
        @inject(FileSystemWatcher) protected readonly fileWatcher: FileSystemWatcher;

and code to put in init:

        this.fileWatcher.onFilesChanged(async changes => {
            for (const change of changes) {
                const uriString = change.uri.toString();
                if (uriString.endsWith('.foo')
                    && (change.type === FileChangeType.ADDED
                        || change.type === FileChangeType.DELETED)) {
                    const fooUri = uriString.substring(0, uriString.length - 4);
                    this.onElementUpdatedEmitter.fire(new URI(fooUri));
                }
            }
        });

which will need this

    get onElementUpdated(): Event<URI> {
        return this.onElementUpdatedEmitter.event;
    }

and the code to get the name could be something like:

   public async getName(element: URI | FileStat): Promise<string> {
         let name = <previous way of getting name>;
         const elementUri = this.getUri(element);
          const elementStat = await this.getStat(element);
          if (elementStat && elementStat.isDirectory) {
            const fooFileUri = `${elementStat.uri}.foo`;
            const fooFileStat = await this.fileSystem.getFileStat(fooFileUri);
            if (fooFileStat) {
                name = name + '-with-foo';
            }
            return name;
    }

Sorry, it's a lot of work to test. Alternatively just test that nothing is broken with this PR which will not require any extra code.

Review checklist

[x ] as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

packages/core/src/browser/label-provider.ts

akosyakov · 2019-08-08T07:17:02Z

LabelProvider is supposed to provide a display name for an element system-wide, not only for the navigator. If you need it only for the navigator why don't require a rerendering of it? Otherwise we need to analyze all existing clients and make sure that they respect new event? Are they really interested in it?

westbury · 2019-08-08T09:28:12Z

If you need it only for the navigator why don't require a rerendering of it?

Surely that would require coupling the label provider to the navigator. In any case rerendering the navigator would not in itself update the icon. We would need to make code changes to expose a function to clear the node in the nodes map Furthermore, if other UI components are developed that cache label properties or show them for long periods then they can listen to the emitter too, avoiding coupling between the various label providers and the various label consumers.

Although the navigator is currently the only place where stale labels are a problem, we would want the icon to be changed system-wide. I assume that would be the case for most uses, as it would defeat the purpose of the names and icons if they were different in different places.

Otherwise we need to analyze all existing clients and make sure that they respect new event

Ideally, yes. However no other uses in the 'theia' repository cached the property values, that I could see. Ideally the trees (markers, callhierarchy), and perhaps resources in debug and the scm views, would update. In our case that would be overkill, but I could see that other developers updating the label properties might want other consumers to show the updates. In that case those developers would be responsible for updating the consumers.

Are they really interested in it?

Most consumers of the labels will not be interested. They don't need to do anything different. The event emitter is there for the few consumers who may be interested.

akosyakov · 2019-08-09T07:10:28Z

Ideally, yes. However no other uses in the 'theia' repository cached the property values, that I could see. Ideally the trees (markers, callhierarchy), and perhaps resources in debug and the scm views, would update.

They are cached as part of React internal virtual DOM state, which should be rerendered for all clients which you mentioned.

Most consumers of the labels will not be interested.

My point is that LabelProvider should return a name which can be used system-wide regardless to a client.

If it returns one name for the navigator, but another name for other clients then it should not be implemented in LabelProvider. For example you can customize NavigatorTree.toNode that it computes name differently and update cached instances on fs events specifically for your case.

akosyakov · 2019-08-09T07:11:07Z

btw LabelProvider.getName is not async, it works by accident in the navigator.

westbury · 2019-08-09T10:40:26Z

If it returns one name for the navigator, but another name for other clients then it should not be implemented in LabelProvider

There seems to be some confusion here. Label providers don't know if they are being called from the Navigator or from elsewhere (and nor should they), so how could it return one name for the navigator but another name for other clients?

We want names and icons to be the same across the application. We do NOT want the icon in the navigator to be different from the icon shown elsewhere. The problem is that currently the icon in the navigator IS different from the icons shown elsewhere. The reason is that a state change resulted in a different icon now being returned by the label provider for a given fixed URI. The navigator shows the old icon, elsewhere we see the new icon (as soon as the icon is rerendered).

We really have two choices:

We say that all label properties must be determinable from the URI (or FileStat) on the very first call. Furthermore label providers cannot use any data source or state that could potentially change, as it needs to guarantee that the label properties returned for a given URI never change.

or 2) We add a emitter or something similar so UI components with cache maps can refresh the cache and, ideally but not so important, other components can choose to rerender.

akosyakov · 2019-08-12T08:05:57Z

There seems to be some confusion here. Label providers don't know if they are being called from the Navigator or from elsewhere (and nor should they), so how could it return one name for the navigator but another name for other clients?

LabelProvide is not responsible to know. It is a responsibility of a client to decide whether to create custom names from URIs or use LabelProvider to get a systemwide name for a URI.

In your case it seems that you want a template method to customize node names for the navigator rather than change a systemwide name.

Just to be clear I'm not against to have an event that a systemwide name is changed, but

it is more work to hook all clients and trigger rerendering to display proper names
it does not seem to be what you are trying to achieve, since you are interested in custom names only for the navigator

westbury · 2019-08-12T10:31:01Z

But we want our custom icon to appear systemwide. That is why we need it to be provided by the label provider.

I would be very happy to make the code changes in the other places. This just was not as important to us as the navigator because our icon was rerendered soon enough anyway.

akosyakov · 2019-08-13T06:31:40Z

@westbury got you, i will compile a list of places to be updated, please look at existing comments for now

packages/core/src/browser/label-provider.ts

packages/filesystem/src/browser/file-tree/file-tree.ts

akosyakov

it looks good, but won't work properly for diff editors.

packages/filesystem/src/browser/file-tree/file-tree.ts

packages/editor/src/browser/editor-widget.ts

packages/core/src/browser/tree/tree.ts

packages/core/src/browser/label-provider.ts

packages/editor/src/browser/editor-widget-factory.ts

packages/workspace/src/browser/workspace-uri-contribution.ts

akosyakov · 2019-10-23T08:32:02Z

@westbury We've discussed at the dev meeting a month ago to have the example Theia extension in Theia repo to provide internal clients for PRs like that. Could you convert the last commit into such extension? It should be located under examples/api-samples, be private and used by browser and electron applications. It should not do anything by default, but provides a command to trigger some sample.

Please rebase this PR as well. For testing I'm going to rebase it myself in Gitpod and verify different LabelProvider clients as is for now.

akosyakov

Should not DiffUriLabelProviderContribution implement onDidChange as well by delegating to LabelProvider.onDidChange in alignment with getLongName and getName methods?

packages/core/src/browser/label-provider.ts

akosyakov · 2019-10-23T09:26:57Z

packages/filesystem/src/browser/file-tree/file-tree.ts

+                    mutableNode.name = this.labelProvider.getName(uri);
+                    mutableNode.description = this.labelProvider.getLongName(uri);
+                    mutableNode.icon = await this.labelProvider.getIcon(uri);
+                    this.fireChanged();


Should be enough to file once after all nodes are processed?

I see that the git history widget respects onDidChange event, but the git diff widget does not. Is there a reason on showing old labels for the diff widget?

This is now covered. Note that we can't simply have the diff label provider 'accepts' function extract the two parts and pass them on. This would cause infinite recursion. I have therefore redesigned this so that the two component uris can be obtained and the recursion handled by LabelProvider.

We could have a flag recursion in GitLabelProviderContribution which set to true before calling LabelProvider again, so it will skip when recursion is true. There is no need to add another API to LabelProviderContribution.

packages/filesystem/src/browser/file-tree/file-tree.ts

akosyakov

Also GitUriLabelProviderContribution does not implement onDIdChange, although relies on the delegation to LabelProvider.

packages/debug/src/browser/debug-session-manager.ts

akosyakov · 2019-10-23T09:53:42Z

I see that the git history widget respects onDidChange event, but the git diff widget does not. Is there a reason on showing old labels for the diff widget?

akosyakov

Props of the mini browser are not updated if they are computed from the label provider. W probably should handle it here: https://github.com/westbury/theia/blob/59b4ad001e6f57cb823dab56a23cdb32a4c50868/packages/mini-browser/src/browser/mini-browser-open-handler.ts#L126 It could be a bit tricky, since we should not do it when name and iconClass provided. It would be good to have a follow-up issue at least.

packages/markers/src/browser/marker-tree.ts

akosyakov · 2019-10-23T10:01:26Z

SCM status bar repository entry is not updated when a label is changed: https://github.com/westbury/theia/blob/59b4ad001e6f57cb823dab56a23cdb32a4c50868/packages/scm/src/browser/scm-contribution.ts#L151

akosyakov

Generally looks and behaves good. But it would be good to add guards to avoid unnecessary rerendering. Also existing label provider contributions should respect new event.

packages/scm/src/browser/scm-widget.tsx

packages/editor/src/browser/editor-widget.ts

akosyakov · 2019-10-23T10:06:21Z

packages/git/src/browser/history/git-history-widget.tsx

+                }
+            }
+        }
+        this.update();


please guard it

the guard is still missing, it fires even if nothing is affected

packages/navigator/src/browser/navigator-tree.ts

westbury · 2019-10-25T21:45:35Z

We've discussed at the dev meeting a month ago to have the example Theia extension in Theia repo to provide internal clients for PRs like that. Could you convert the last commit into such extension?

OK, I've done that. Perhaps you wanted just a single extension called api-samples instead of an extension under api-samples directory just for label providers? I should have been at the Tuesday dev meeting, then I would know. Anyway, let me know if this is the structure you were expecting.

akosyakov · 2019-10-26T06:02:12Z

OK, I've done that. Perhaps you wanted just a single extension called api-samples instead of an extension under api-samples directory just for label providers?

Yes, I meant a single @theia/api-samples extension. Sorry for the confusion.

akosyakov · 2019-11-20T16:31:49Z

@westbury I need these changes to support icon theming VS Code contribution point. Would it be fine if i integrate your commits in my PR? or you are planning to work on it soon?

westbury · 2019-11-20T16:41:50Z

@westbury I need these changes to support icon theming VS Code contribution point. Would it be fine if i integrate your commits in my PR? or you are planning to work on it soon?

Sorry to be so long before getting back to this. I didn't think anyone else was really interested. I have more changes so I will push those shortly.

westbury · 2019-11-20T23:44:30Z

Props of the mini browser are not updated if they are computed from the label provider.
I've created #6596 to cover this.

westbury

Other than where I have commented above, I believe the issues have been addressed.

akosyakov · 2019-11-21T09:27:28Z

Other than where I have commented above, I believe the issues have been addressed.

Thank you. Trying it out!

akosyakov · 2019-11-21T10:13:00Z

packages/core/src/browser/label-provider.ts

+     */
+    readonly onDidChange?: Event<DidChangeLabelEvent>;
+
+    readonly getConstituentUris?: (compositeUri: URI) => URI[];


I thought we agree that URI should not leak here. Could we remove it and use only onDidChange? Implementation should check whether passed element in affects actually is URI. We would like to pass other objects like FileStat and any other shapes having uri property.

Unfortuntely one cannot implement this in onDidChange using the obvious way of putting something like the following in the DiffUriLabelProviderContribution constructor:
`
labelProvider.onDidChange(event => {

this.onDidChangeEmitter.fire({ affects: (element: URI) => { if (!(element instanceof URI && DiffUris.isDiffUri(element))) { return false; } const label = element.path.toString(); if (label) { return false; } const [left, right] = DiffUris.decode(element); return event.affects(left) || event.affects(right); } });

});
`
The problem is that the code that fires the event does not know what URIs it may get. It could potentially get a diff URI in which one of the nested URIs is also a diff URI, down any number of levels. This above code will recurse indefinitely as it fires an event for a one-level nested diff URI, then an event for a two level nested diff URI etc. There are ways of using flags to stop this, given that we know consumers will never nest diff URIs. However this felt to me more like a hack than a generic solution. So I came up with the current implementation which I feel is more robust in the sense that it is harder for contribution providers to make it all go horribly wrong.

akosyakov · 2019-11-21T10:25:31Z

packages/core/src/browser/label-provider.ts

+    protected affects(element: URI, event: DidChangeLabelEvent, originatingContribution: LabelProviderContribution): boolean {
+
+        const contribs = this.findContribution(element);
+        const possibleContribsWithDups = [


Why cannot we just iterate contribs? What would be the point to have a label provider contribution doing nothing? Also does not it break priority order applied in findContribution?

We don't want to process all contributions, just the one that was actually used to obtain the three label properties. As a contribution can implement only one or two of the three property getters, a different contribution may be used for each property. So there could be anywhere from one to three contributions that were actually used to get a property.

'contribs' is all contributions that 'can' process element, in priority order.
'possibleContribsWithDups' contains, in the first element, the actual contribution that would be called to get the icon. This matches the code in getIcon. Similarly the second element for the 'name' property and the third element for the 'longName' property.
'possibleContribsWithoutDups' contains the contributions that are actually called to get one or more of the three properties. So these are the only contributions that we need ask.

akosyakov · 2019-11-21T10:25:53Z

packages/core/src/browser/label-provider.ts

+     * @param uri
+     * @param event
+     */
+    protected affects(element: URI, event: DidChangeLabelEvent, originatingContribution: LabelProviderContribution): boolean {


element should be of object type

akosyakov · 2019-11-21T10:27:46Z

examples/api-samples/README.md

@@ -0,0 +1,9 @@
+# Theia - Examples - API Samples - Label Provider


We should add a new extension to top-level tsconfig.json. Otherwise reference search does not work for it. Try to find who implement LabelProviderContribution.didChange

akosyakov · 2019-11-21T10:28:24Z

examples/api-samples/src/browser/dynamic-label-provider-contribution.ts

+import { Emitter, Event } from '@theia/core';
+
+@injectable()
+export class DynamicLabelProviderContribution extends DefaultUriLabelProviderContribution {


Could we prefix it with Sample?

packages/debug/src/browser/debug-session-manager.ts

packages/editor/src/browser/editor-widget.ts

akosyakov · 2019-11-21T10:34:38Z

packages/git/src/browser/history/git-history-widget.tsx

+                }
+            }
+        }
+        this.update();


the guard is still missing, it fires even if nothing is affected

akosyakov · 2019-11-21T10:42:37Z

Please commit modification to .travis.yml file otherwise build is failing with:

HEAD detached at FETCH_HEAD
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)
	modified:   .travis.yml

akosyakov · 2019-11-21T10:43:15Z

Let me know if you don't have time, I could clean up my concerns with the additional commit.

westbury · 2019-11-21T14:01:20Z

@akosyakov I've addessed all your comments except your concerns about getConstituentUris (as I'm not sure what we can do that is better there).

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

akosyakov · 2019-11-21T15:24:00Z

@westbury I will have a look again now. I'm thinking to go with your proposal and see how it works when I need to change icon theme. I won't consider getConstituentUris as final for now, but reconsider it to something more abstract from uri later. ok?

westbury · 2019-11-21T15:49:27Z

I won't consider getConstituentUris as final for now, but reconsider it to something more abstract from uri later. ok?

That's fine. I understand why you don't like it. I am interested in seeing what you come up with instead.

akosyakov

works nicely from user perspective, thank you for taking care of all internal clients! ❤️

akosyakov · 2019-11-22T10:17:39Z

@westbury i'm merging it to continue working on icon theme support

akosyakov · 2019-11-22T16:38:15Z

I was to fast to merge, we've overlooked DI infinity loop on the startup, logs are full of errors on master now:

…butions See #5884 (comment) Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

…butions See eclipse-theia#5884 (comment) Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

westbury commented Aug 7, 2019

View reviewed changes

packages/core/src/browser/label-provider.ts Show resolved Hide resolved

akosyakov added navigator issues related to the navigator/explorer shell issues related to the core shell labels Aug 8, 2019

akosyakov reviewed Aug 13, 2019

View reviewed changes

westbury force-pushed the label-updates branch 2 times, most recently from a48e7e2 to ae65026 Compare August 20, 2019 09:17

akosyakov reviewed Aug 20, 2019

View reviewed changes

packages/workspace/src/browser/workspace-uri-contribution.ts Outdated Show resolved Hide resolved

akosyakov reviewed Aug 20, 2019

View reviewed changes

packages/workspace/src/browser/workspace-uri-contribution.ts Outdated Show resolved Hide resolved

westbury force-pushed the label-updates branch from ae65026 to 59b4ad0 Compare September 11, 2019 16:20

akosyakov reviewed Oct 23, 2019

View reviewed changes

packages/debug/src/browser/debug-session-manager.ts Show resolved Hide resolved

akosyakov reviewed Oct 23, 2019

View reviewed changes

packages/markers/src/browser/marker-tree.ts Outdated Show resolved Hide resolved

akosyakov reviewed Oct 23, 2019

View reviewed changes

westbury force-pushed the label-updates branch from 59b4ad0 to fbadf73 Compare October 25, 2019 21:36

westbury force-pushed the label-updates branch from fbadf73 to bb8d685 Compare November 20, 2019 23:18

westbury mentioned this pull request Nov 20, 2019

Label updates are not refreshed in mini browser props #6596

Open

westbury commented Nov 20, 2019

View reviewed changes

akosyakov reviewed Nov 21, 2019

View reviewed changes

westbury force-pushed the label-updates branch from bb8d685 to 51161d2 Compare November 21, 2019 12:23

Allow label providers to notify that changes require a refresh

b65c3d9

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>

westbury force-pushed the label-updates branch from 51161d2 to b65c3d9 Compare November 21, 2019 15:21

akosyakov approved these changes Nov 22, 2019

View reviewed changes

akosyakov merged commit 2938cd0 into eclipse-theia:master Nov 22, 2019

westbury deleted the label-updates branch November 22, 2019 11:17

akosyakov added a commit that referenced this pull request Nov 22, 2019

break dependency injection cycle between LabelProvider and its contri…

3cf6cf5

…butions See #5884 (comment) Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov mentioned this pull request Nov 22, 2019

break DI cycle between LabelProvider and its contributions #6608

Merged

1 task

akosyakov added a commit that referenced this pull request Nov 23, 2019

break dependency injection cycle between LabelProvider and its contri…

145f913

…butions See #5884 (comment) Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov added a commit to akosyakov/theia that referenced this pull request Feb 24, 2020

break dependency injection cycle between LabelProvider and its contri…

36fe88c

…butions See eclipse-theia#5884 (comment) Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

		@@ -0,0 +1,9 @@
		# Theia - Examples - API Samples - Label Provider

Allow label providers to notify that changes require a refresh #5884

Allow label providers to notify that changes require a refresh #5884

Conversation

westbury commented Aug 7, 2019

How to test

Review checklist

Reminder for reviewers

akosyakov commented Aug 8, 2019

westbury commented Aug 8, 2019

akosyakov commented Aug 9, 2019

akosyakov commented Aug 9, 2019

westbury commented Aug 9, 2019

akosyakov commented Aug 12, 2019 • edited Loading

westbury commented Aug 12, 2019

akosyakov commented Aug 13, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Oct 23, 2019 • edited Loading

akosyakov left a comment • 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

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Oct 23, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Oct 23, 2019

akosyakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westbury commented Oct 25, 2019

akosyakov commented Oct 26, 2019 • edited Loading

akosyakov commented Nov 20, 2019 • edited Loading

westbury commented Nov 20, 2019

westbury commented Nov 20, 2019

westbury left a comment

Choose a reason for hiding this comment

akosyakov commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westbury Nov 21, 2019 • 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

akosyakov commented Nov 21, 2019

akosyakov commented Nov 21, 2019

westbury commented Nov 21, 2019

akosyakov commented Nov 21, 2019

westbury commented Nov 21, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Nov 22, 2019

akosyakov commented Nov 22, 2019

akosyakov commented Aug 12, 2019 •

edited

Loading

akosyakov commented Oct 23, 2019 •

edited

Loading

akosyakov left a comment •

edited

Loading

akosyakov commented Oct 26, 2019 •

edited

Loading

akosyakov commented Nov 20, 2019 •

edited

Loading

westbury Nov 21, 2019 •

edited

Loading