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

preferences: make schemas extensible #9883

Merged
merged 1 commit into from
Aug 19, 2021
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Aug 11, 2021

What it does

The pull-request allows adopters the ability to more easily rebind preference schemas.

This allows extenders to easily modify preference schemas as illustrated in the commit c5a7d71.

How to test

  1. ensure that the application successfully builds and starts.
  2. start the application, and navigate to the preferences-view - preferences should be correctly listed.
  3. search for workspace.preserveWindow, the preference definition should not be listed as it was removed in c5a7d71 for test purposes.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added preferences issues related to preferences extensibility issues to simplify ability to extend Theia labels Aug 11, 2021
@vince-fugnitto vince-fugnitto self-assigned this Aug 11, 2021
@vince-fugnitto vince-fugnitto force-pushed the vf/preferences-schema branch 2 times, most recently from b8a90b6 to 77ec9fb Compare August 11, 2021 17:50
@vince-fugnitto
Copy link
Member Author

@paul-marechal @colin-grant-work I refactored and simplified the pull-request to use Symbol instead when binding 👍

@vince-fugnitto vince-fugnitto force-pushed the vf/preferences-schema branch 2 times, most recently from 7636170 to 8ca1b62 Compare August 12, 2021 13:04
@westbury
Copy link
Contributor

@vince-fugnitto thanks for resurrecting this issue about overriding preferences. We've have been using a patched version of Theia for quite some time now so that we can overwrite preferences.

This is a good and simple way of allowing extenders to override preferences and would support a lot of our use cases. My main concerns are:

  1. I don't think this will support the override of preferences that are provided by plugins.
  2. This puts responsibility on the extender to ensure that modifications to the preferences do not break the requirements of the code. If changes to the preference schema result in preferences that do not match what is in the interface (eg WorkspaceConfiguration) then we could have problems. For example, the code will assume that a preference exists and there must be a default value that is returned even if the preference has been 'removed'. Perhaps this is ok.
  3. We cannot remove a preference from the UI but still allow it to be changed programmatically, unless I am missing something.

If you prefer this approach to the approach in #8626 that is fine but I will then need to amend that PR so that we can make our changes to preferences in plugins.

@vince-fugnitto
Copy link
Member Author

@westbury thank you for the discussion, in the end I believe the change would be something like the two pull-requests combined (easier to rebind, possibility to hide).

  1. I don't think this will support the override of preferences that are provided by plugins.

Correct, the pull-request aims at making the rebinding of individual PreferenceContribution more extensible from @theia extensions. There was a question in our community regarding the difficulties of rebinding a schema which also required additional workarounds to rebind the PreferenceContribution:

  1. This puts responsibility on the extender to ensure that modifications to the preferences do not break the requirements of the code. If changes to the preference schema result in preferences that do not match what is in the interface (eg WorkspaceConfiguration) then we could have problems. For example, the code will assume that a preference exists and there must be a default value that is returned even if the preference has been 'removed'. Perhaps this is ok.

I believe references to preferences should be fine, the preference-service will handle getting preference values which do not exist, returning undefined. If there happens to be issues, I believe it would be the responsibility of the application or extension developer to handle them.

  1. We cannot remove a preference from the UI but still allow it to be changed programmatically, unless I am missing something.

The possibility to hide a preference would be achieved by your pull-request I believe (only UI). I add the possibility of modifying the schema altogether in a downstream extension. If a preference no longer exists it will of course not be visible, or editable by end-users.

@vince-fugnitto vince-fugnitto force-pushed the vf/preferences-schema branch 2 times, most recently from ba4fbf7 to 3a65ac3 Compare August 19, 2021 12:14
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

The refactoring is minimal and seems to make sense.

@colin-grant-work
Copy link
Contributor

The possibility to hide a preference would be achieved by your pull-request I believe (only UI). I add the possibility of modifying the schema altogether in a downstream extension. If a preference no longer exists it will of course not be visible, or editable by end-users.

@westbury is correct that his PR has this feature which this PR does not - keeping a preference alive for programmatic manipulation but (somewhat) hidden from users - but I don't think that's an argument against this PR. That is a separate feature that can be implemented on top of this PR.

@vince-fugnitto
Copy link
Member Author

I dropped the test commit (which performs the rebind for test-purposes). I'll merge once CI is green 👍

The commit refactors the binding of preference schemas so they can be
easily rebinding by downstream extenders and applications if necessary.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto merged commit b23360d into master Aug 19, 2021
@vince-fugnitto vince-fugnitto deleted the vf/preferences-schema branch August 19, 2021 18:42
@github-actions github-actions bot added this to the 1.17.0 milestone Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility issues to simplify ability to extend Theia preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants