Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch schema change event in order to have actual representation of user preferences #6510

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Nov 7, 2019

What it does

This changes proposal fixes the problem, when user preference tree wasn't respect preference schema change event. The problem was in initializer code fragment that collect combined preference scheme during tree create. Here is a sample of plugin, that has definition of preference in package.json. The flow of representing problem. HostedPluginSupport initialize and calls load function and then doLoad where loads contribution and handle it, calling this function. Then it takes contribution configuration and operates with scheme, setting it up into preference provider where preference changed event fires and nothing appears. So there is a reason to subscribe preference tree listen to this kind of event to be able to update the representation of preference tree when schema changes.

There is a linked issue for this problem: eclipse-che/che#15001

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

How to test

Clone this sample plugin, build it and place in theia/plugin directory and start theia. In master branch there is a problem, when user refreshes the page where opened preference editor and after refresh configuration from the custom contribution disappears.

Review checklist

Reminder for reviewers

@vzhukovs vzhukovs added bug bugs found in the application preferences issues related to preferences plug-in system issues related to the plug-in system Team: Che-Plugins issues related to the che-plugins team labels Nov 7, 2019
@vzhukovs vzhukovs self-assigned this Nov 7, 2019
@akosyakov
Copy link
Member

We should test that selection and expansion are preserved when schema is changed.

@akosyakov
Copy link
Member

@vzhukovskii please squash follow-up fixes for code clean ups and issues introduced by a PR itself in the original commit: https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#checklist-commit-history

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested using sample plugin from How to test section - the issue is fixed for me: plugin preferences are displayed after page refreshing.

@RomanNikitenko
Copy link
Contributor

btw, from my point of view it makes sense to check this fix for #6467
@lmcbout

…ser preferences

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs
Copy link
Contributor Author

Changes squashed into single commit.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can merge if noone objects and CI is green

@vzhukovs vzhukovs merged commit a8f750a into master Nov 12, 2019
@vzhukovs vzhukovs deleted the user_preferences branch November 12, 2019 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application plug-in system issues related to the plug-in system preferences issues related to preferences Team: Che-Plugins issues related to the che-plugins team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants