Make preference node rendering customizable and provide file selection #10766

martin-fleck-at · 2022-02-17T10:46:47Z

What it does

Fixes #10765

Introduce preference node render service to support contributions
- Convert previous renderer factory content to default contribution
- Let new renderer factory implementation use service
- Contributions are ranked similar to open handlers
Provide base for leaf node contributions and render hints
Implement concrete string renderer for single file selection

How to test

Specify rendering hint in preferences
Usage Example: Windows terminal (part of the commit but can be removed if unwanted)

Review checklist

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

colin-grant-work

Functionally, this appears to work well. I've made a number of comments, mainly about naming, as I think the names currently used don't make very clear what problem is being addressed. Specifically, I think RenderHint and associated names could be made more generic and accurate if changed to something like TypeDetails, and BrowseNode and associated names would be clearer as FilepathNode etc.

Another detail of the implementation worth considering is whether to use a ContributionProvider - whose members generally have to be registered at build time - or a registry that permits runtime extension.

colin-grant-work · 2022-02-17T23:16:38Z

packages/external-terminal/src/electron-browser/external-terminal-preference.ts

@@ -87,6 +87,7 @@ export async function getExternalTerminalSchema(externalTerminalService: Externa
        properties: {
            'terminal.external.windowsExec': {
                type: 'string',
+                renderHint: { rendering: 'browse' },


Regarding most of the names including 'browse,' I think it would makes sense to change to something that refers to something in the semantic field of file / path / folder, for example, in this case:

type: 'string' typeDetails: { isFilepath: true, selectionProps: ...OpenFileDialogProps }

I renamed the fields to better match the file/file path terminology.

packages/preferences/src/browser/views/components/preference-browse-input.ts

packages/preferences/src/browser/views/components/preference-node-renderer-service.ts

packages/preferences/src/browser/views/preference-widget-bindings.ts

packages/preferences/src/browser/views/components/preference-node-renderer-service.ts

martin-fleck-at · 2022-03-04T17:20:25Z

@colin-grant-work Thank you very much for your detailed review! Unfortunately it took me some time to come back to this but I followed your suggestions which were all great in my opinion. The new change looks much cleaner now (even though it touches more code).

colin-grant-work

This is just about there. I have only one substantive comment on the Registry implementation. I traced the performance of the handleDisplayChange method on master and with this code, and despite the increase in overhead picking a renderer for each node, there was only a very small increase in the time taken to fully render (0.1 seconds; 0.5sec vs. 0.4sec previously). A small price to pay for a considerable increase in flexibility. 👍

colin-grant-work · 2022-03-11T15:57:12Z

packages/external-terminal/src/electron-browser/external-terminal-preference.ts

 import {
-    createPreferenceProxy,
-    PreferenceSchema,
-    PreferenceService,
-    PreferenceProxy
+    createPreferenceProxy, PreferenceProxy, PreferenceSchema,
+    PreferenceService
 } from '@theia/core/lib/browser';
 import { PreferenceSchemaProvider } from '@theia/core/lib/browser/preferences/preference-contribution';
-import { isWindows, isOSX } from '@theia/core/lib/common/os';
-import { ExternalTerminalService, ExternalTerminalConfiguration } from '../common/external-terminal';
 import { nls } from '@theia/core/lib/common/nls';
+import { isOSX, isWindows } from '@theia/core/lib/common/os';
+import { inject, injectable, interfaces, postConstruct } from '@theia/core/shared/inversify';
+import { ExternalTerminalConfiguration, ExternalTerminalService } from '../common/external-terminal';


It looks like these changes are probably the work of an auto-formatter of some kind? Since there isn't currently a standard for the repo for the organization of imports (perhaps there should be), it's likely better to turn such auto-formatting off for the time being to limit the diff to relevant changes.

You are right, sorry for that! I restored the original formatting which I usually do when I check the commit but seems like this slipped through ;-)

colin-grant-work · 2022-03-11T16:08:00Z

packages/preferences/src/browser/views/components/preference-file-input.ts

+            this.setPreference(selection);
+        }
+    }
+
+    protected setPreference(uri: URI): void {
+        this.interactable.value = uri.path.toString();
+        this.setPreferenceImmediately(uri.path.toString());
+    }


Minor: since these classes already have two setPreference methods, I think it would be preferable to just call setPreferenceImmediately directly on line 97 rather than introducing a third method with that prefix.

I adapted the method to override setPreferenceImmediately instead and now call that directly. You are right that having 3 methods with similar functionality is definitely curios.

colin-grant-work · 2022-03-11T17:35:15Z

packages/preferences/src/browser/views/components/preference-node-renderer-creator.ts

+export interface PreferenceNodeRendererCreatorRegistry {
+    registerPreferenceNodeRendererCreator(registry: PreferenceNodeRendererCreator): Disposable;
+    unregisterPreferenceNodeRendererCreator(registry: PreferenceNodeRendererCreator): void;
+    getPreferenceNodeRendererCreator(node: Preference.TreeNode): PreferenceNodeRendererCreator;
+}


The registry should have an Emitter/Event to signal that its contents have changed so that users of registry (PreferenceEditorWidget, in this case) can subscribe and react to changes appropriately.

I added an event to the interface and implementation but just a simple one for change and no further details (e.g., added or removed creators), I hope that is fine. Since the PreferenceEditorWidget does not directly use that I was not sure how to apply it but I think having it is definitely a valuable contribution.

@martin-fleck-at, I believe the place to refer to the event would be in the PreferenceEditorWidget class. It should trigger a total repopulation of the renderers field analagous to what happens when the widget starts up.

@colin-grant-work Ah yes, I found it! Somehow I missed that the renderers are cached in the widget so thank you for noticing! I now remove all the cached renderers and re-render the widget as expected. I made a small test commit on a separate branch that toggles the file contribution in case you want to test it: https://github.com/martin-fleck-at/theia/commit/2c4e17b69d9d84f0a4fb586e681da2002187b731

martin-fleck-at · 2022-03-13T18:48:32Z

@colin-grant-work Thank you very much for your detailed review and the rendering performance measurement, that is very interesting and good to know! I already squashed the commits now on top of master but if there is anything else, I'll gladly add it.

- Introduce preference node render registry to support contributions -- Convert previous renderer factory content to default contribution -- Let new renderer factory implementation use service -- Contributions are ranked similar to open handlers - Add 'typeDetails' as generic optional property to preference schema -- Contributions may use type details for further properties - Provide base for leaf node contributions and render hints - Implement concrete string renderer for single file selection -- Usage Example: Windows terminal (external and internal)

colin-grant-work

Looks good to me. I tested with the supplied test commit, and the preference was accurately rerendered. 👍

vince-fugnitto added preferences issues related to preferences proposal feature proposals (potential future features) labels Feb 17, 2022

colin-grant-work self-requested a review February 17, 2022 22:46

colin-grant-work reviewed Feb 18, 2022

View reviewed changes

colin-grant-work self-requested a review March 10, 2022 15:57

colin-grant-work reviewed Mar 11, 2022

View reviewed changes

colin-grant-work self-requested a review March 15, 2022 14:02

colin-grant-work approved these changes Mar 25, 2022

View reviewed changes

JonasHelming merged commit 7b8d3b7 into eclipse-theia:master Mar 30, 2022

w-pearson mentioned this pull request Jun 1, 2023

Don't cover input field with file preference browse button #12586

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make preference node rendering customizable and provide file selection #10766

Make preference node rendering customizable and provide file selection #10766

martin-fleck-at commented Feb 17, 2022

colin-grant-work left a comment

colin-grant-work Feb 17, 2022

martin-fleck-at Mar 4, 2022

martin-fleck-at commented Mar 4, 2022

colin-grant-work left a comment

colin-grant-work Mar 11, 2022

martin-fleck-at Mar 13, 2022

colin-grant-work Mar 11, 2022

martin-fleck-at Mar 13, 2022

colin-grant-work Mar 11, 2022

martin-fleck-at Mar 13, 2022

colin-grant-work Mar 18, 2022

martin-fleck-at Mar 20, 2022

martin-fleck-at commented Mar 13, 2022

colin-grant-work left a comment

Make preference node rendering customizable and provide file selection #10766

Make preference node rendering customizable and provide file selection #10766

Conversation

martin-fleck-at commented Feb 17, 2022

What it does

How to test

Review checklist

Reminder for reviewers

colin-grant-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at commented Mar 4, 2022

colin-grant-work 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

martin-fleck-at commented Mar 13, 2022

colin-grant-work left a comment

Choose a reason for hiding this comment