Added Preferences UI widget #7105

NicholasStenbeck · 2020-02-07T17:03:35Z

Signed-off-by: Nicholas Stenbeck nicholas.stenbeck@ericsson.com

What it does

This package adds a UI view to Preferences. It is a work in progress and is meant as a reference to get community approval, so that we can continue work in this direction. All feedback is welcome.
Addresses #6985.

Features

Updates UI Similar to VS Code
Fuzzy search to filter preferences
Jump to category from navbar on the side
Revert to default option from context menu
Copy JSON code for preference from context menu

How to test

Go to file > settings > Open Preferences (UI).
Change a preference.
Go to file > settings > Open Preferences (JSON).
Confirm that the change is there.
Try writing something in the Preferences UI search bar and make sure that the preferences are filtered properly.
Click a category in the Preferences UI navbar and confirm that the Preferences UI widget scrolls to the right category.

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

vince-fugnitto

Thank you for your contribution @NicholasStenbeck! 👍
There are unfortunately some build errors at the moment (CI is currently failing) which prevents the project from successfully verifying the functionality of the changes.

paul-marechal

Like @vince-fugnitto said, CI is red, and I wasn't able to build NicholasStenbeck:preferences-ui locally either. Not sure what went wrong.

packages/core/src/common/common-frontend-contribution.ts

packages/preferences/src/browser/UI/settings-ui-plus-array.tsx

packages/preferences/src/browser/UI/settings-ui-plus-editor.tsx

packages/preferences/src/browser/UI/settings-ui-plus-tree.ts

akosyakov · 2020-02-10T05:58:13Z

Please make a PR as minimal as possible and avoid touching unrelated code, all new code should be aligned with our coding guidelines: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines Please ping when it is done, i.e. look at naming, styling, theming, check keyboard support, align it as much as possible with existing code based with minimal breaking changes then it would easy to accept it.

akosyakov · 2020-02-10T06:31:19Z

@danieltomasku @NicholasStenbeck if you won’t have time to work on it, please let us know that someone can finish it up.

akosyakov · 2020-02-10T06:33:10Z

Important thing that we should use trees in the implementation if vscode does it, to avoid getting in the same maintenance troubles as we get with scm, i.e. trees will be improved to catch up with vscode and then if some feature is enabled for preference tree in vscode, it should be enabled the same way for Theia.

NicholasStenbeck · 2020-02-10T13:51:42Z

@akosyakov Thanks for the input! I've addressed some issues in the code for an update. Could you elaborate on the comment about using trees? As far as I know, we use pretty much the same method of populating the preferences widget as the current one. I didn't write that part of the package, but @SammyGP might have a better grasp of it.

SammyGP · 2020-02-10T14:42:26Z

@akosyakov Thanks for the input! I've addressed some issues in the code for an update. Could you elaborate on the comment about using trees? As far as I know, we use pretty much the same method of populating the preferences widget as the current one. I didn't write that part of the package, but @SammyGP might have a better grasp of it.

I'm not sure how vscode does it behind the curtains but the way we implemented the tree we could make some minor changes to make it more visually aligned with how vscode does it.

The way its decoupled from the text view it should be safe to add features to the tree without to much trouble.

packages/core/src/browser/common-frontend-contribution.ts

packages/preferences/src/browser/ui/settings-ui-plus-widget.tsx

packages/preferences/src/browser/ui/settings-ui-plus-navbar.tsx

packages/preferences/src/browser/ui/settings-ui-plus-fuzzy-search.ts

packages/preferences/src/browser/style/search-input.css

packages/preferences/src/browser/style/preference-object.css

packages/preferences/src/browser/preference-frontend-module.ts

NicholasStenbeck · 2020-02-14T14:04:51Z

I have pushed an update trying to address most issues mentioned by @akosyakov. I am planning on looking into removing the unnecessary files under preferences/ and adding a way to open a JSON-file with settings in the code editor from the preferences UI.

packages/core/src/browser/common-frontend-contribution.ts

akosyakov · 2020-05-01T07:04:03Z

It would be good to have follow-up issues for all points. Regarding context menu, ContextMenuRenderer makes sure that there is always only one context menu visible, if we work around such service will lose some logic.

akosyakov

Thanks for reverting irrelevant changes ❤️
Please have a look at comments below and #7105 (comment). Only core extension can style global elements, other extension must not.

packages/preferences/src/browser/preferences-decorator.ts

akosyakov · 2020-05-01T07:27:10Z

packages/preferences/src/browser/style/index.css

+  --theia-settingsSticky-height: 100px;
+}
+
+html,


please get rid of styles to global element like html and body

packages/preferences/src/browser/views/components/preference-boolean-input.tsx

packages/preferences/src/browser/views/components/preference-string-input.tsx

packages/preferences/src/browser/views/components/single-preference-wrapper.tsx

packages/preferences/src/browser/views/preference-tree-widget.tsx

colin-grant-work · 2020-05-01T12:49:03Z

Many thanks for your comments. We will work to address them.

Co-authored-by: Nicholas Stenbeck <nicholas.stenbeck@ericsson.com> Co-authored-by: Colin Grant <colin.grant@ericsson.com> Co-authored-by: Kenneth Marut<kenneth.marut@ericsson.com> Signed-off-by: Nicholas Stenbeck <nicholas.stenbeck@ericsson.com> Signed-off-by: Colin Grant <colin.grant@ericsson.com> Signed-off-by: Kenneth Marut <kenneth.marut@ericsson.com>

kenneth-marut-work · 2020-05-01T22:57:21Z

@akosyakov

We've just pushed another update that has addressed your comments:

Regarding context menu, ContextMenuRenderer makes sure that there is always only one context menu visible, if we work around such service will lose some logic.

The context menu generation for the gear-icon and folder-tab has been refactored to use ContextMenuRenderer. We have noticed odd behavior in the core's electron-context-menu-renderer.ts where the values provided for the anchor are ignored, which causes the context menu to render at the location of the click, rather than the passed-in value. We will file an issue for this. This behavior is more noticeable for the folder-tab context-menu.

please get rid of styles to global element like html and body

Done

(nit) public is default, you don't need to type it

We have removed all mentions of public

it violates https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#no-bind-fn-in-event-handlers and break react caches. I've seen the same happening in other components.
please review all react components that you never use arrow functions, that's perf killer

We've extracted all arrow functions into class members where possible. We have used the React hook useCallback in functional components to prevent redeclaration of functions during rerenders.

(minor) https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#property-injection

In preference-tree-widget.tsx We have updated the constructor injection to property injection and moved all constructor logic to an @postConstruct function.
We noticed that this was also an issue in preferences-decorator.ts and have updated it as well. Please let us know if you would like this to be reverted.

Additionally we have looked at the coding-guidelines and updated any references to URI.displayName to use the LabelProvider.getName method instead.

akosyakov

It works nicely for me.

I've noticed that the widget state is not preserved on reload like the search input. But It can be fixed as a follow-up issue. It could be also good idea to check whether this widget is not leaking by closing it, opening other widgets, triggering GC, taking a snapshot and checking whether everything related to preference UI is garbage collected.

akosyakov · 2020-05-05T12:27:26Z

packages/preferences/src/browser/preference-tree-provider.ts

+        (options: PreferenceFilterOptions, newScope?: Preference.SelectedScopeDetails) => this.updateUnderlyingData(options, newScope),
+        200
+    );
+    protected handleSearchChange = debounce((term: string) => this.updateDisplay(term), 100);


Yes, this ended up going unused. Instead, the search event fire is debounced. Shall we remove it now before this gets merged?

akosyakov · 2020-05-05T12:28:38Z

packages/preferences/src/browser/preferences-decorator.ts

-                    }]
-            }] as [string, TreeDecoration.Data];
-        }));
+        if (preferences) {


what is a guard for? preferences are always defined

@NicholasStenbeck, I think you introduced this check. Can you shed some light here?

akosyakov · 2020-05-05T12:41:21Z

@vince-fugnitto @lmcbout Would you have a look or we wait till tomorrow and merge?

vince-fugnitto · 2020-05-05T12:42:41Z

@vince-fugnitto @lmcbout Would you have a look or we wait till tomorrow and merge?

I can have a look at the moment, we can merge tomorrow anyways and fix any issues/improvements we might encounter during the month.

vince-fugnitto

The changes look great, it's a very nice improvement over the previous implementation!
I've opened some follow-up issues that we can tackle once the pull-request is merged :)

akosyakov · 2020-05-06T07:26:49Z

@vince-fugnitto please merge

NicholasStenbeck changed the title ~~Added Preferences UI widget~~ WIP: Added Preferences UI widget Feb 7, 2020

NicholasStenbeck requested review from akosyakov, vince-fugnitto, marcdumais-work and paul-marechal February 7, 2020 17:05

marcdumais-work mentioned this pull request Feb 7, 2020

Possible contribution for a new preference editor #6985

Closed

vince-fugnitto reviewed Feb 7, 2020

View reviewed changes

vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves preferences issues related to preferences labels Feb 7, 2020

paul-marechal reviewed Feb 7, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

NicholasStenbeck force-pushed the preferences-ui branch from 115d3e1 to 897152c Compare February 10, 2020 13:19

This comment has been minimized.

Sign in to view