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

Allow extenders to modify preferences #8626

Closed
wants to merge 1 commit into from
Closed

Conversation

westbury
Copy link
Contributor

What it does

These changes add the ability for packages to modify the preference schema for preferences added by others. This allows extenders to make modifications to the schema for preferences added either by other packages or by plugins. See https://community.theia-ide.org/t/hidden-settings/821 .

An additional property has been added to the preference schema: 'hidden'. This allows extenders to remove a preference altogether from the UI. It won't show in the preference view nor will it be included in the counts shown in that view.

Note that when a preference is created, the code reading that preference is going to assume that the preference exists and that the value meets the constraints specified in the schema. Therefore the preference is still there, even if hidden. Furthermore changes cannot violate the original schema. The following changes are allowed:

  • the description can be changed.
  • if an enum, the enum list can be replaced by a subset. However extra values cannot be added to the enum because that might break readers of the preference.
  • if a string preference then an enum can be set to limit the string to certain values
  • a minimum value can be set for numbers. However if the preference already has a minimum then the new minimum cannot be less.
  • the 'hidden' flag can be turned on or off.
  • the default value can be changed (note that this can also be changed in packages.json)

How to test

Checkout branch modifying-prefs-test. This branch contains an extra commit with various preferences modified.

Some of these modifications are to preferences in the git built-in plugin. Start up without the git plugin, then install it. You will see the plugin's preferences but with the modifications. Uninstall the plugin and its preferences disappear. Install the plugin and they come back again with the modifications.

Modifications always override the original preference schema, even if there is no package dependency. However package dependency does prioritize appropriately where multiple packages are modifying the same preference.

Review checklist

Reminder for reviewers

@westbury westbury added the preferences issues related to preferences label Oct 13, 2020
@@ -76,6 +76,7 @@ export interface PreferenceItem {
additionalProperties?: object | boolean;
[name: string]: any;
overridable?: boolean;
hidden?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

@westbury the property is used to hide elements in the view but it still allows users to add the preference in their settings (with or without the use of auto-completion). Is there a reason you want to hide the preference rather than make it non-editable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason you want to hide the preference rather than make it non-editable?

Consider a custom IDE that is targeted to a very specific set of users using a specific tool chain for a specific purpose. There will be preferences that must be set to a particular value and setting the preference to any other value will always break the user. For example, a couple of days ago, after I had put in this PR, we had a bug report from a user. They had set on the 'Support Multi Root Workspace' preference. Our IDE works to a specific directory layout and does not support multi-root workspaces. So, in this situation, should we show the preference but make it non-editable, or not show it? I would suggest that not showing it is a better user experience. Mentioning 'multi-root workspaces' in a custom IDE that has no concept of multi-root workspaces will only cause confusion. If users know what multi-root workspaces are then they will think they must be supported and send in support requests complaining that they can't get them to work. A large part of providing a clean IDE is that we do not expose stuff that does not apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should be removed from auto-complete in settings too.

Copy link
Member

Choose a reason for hiding this comment

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

@westbury I'm not arguing the use-case, in fact I believe it is something I brought up in the past: https://spectrum.chat/theia/general/is-there-a-need-to-set-specific-preferences-as-non-overridable~2cf3bb68-59b2-4f4b-8070-b2839c35ba4d.

The issue with the approach is while the preference is hidden, the preference is still editable which can still break applications (for instance if they do not expect to support multi-root workspaces).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how we can stop the user from editing the settings file. If the requirement is that we do not want to give the user any way of changing the preference (short of hacking javascript) then we would have to ignore any value in the settings file, always using the default value. That would provide a consistent approach but it does remove the ability to change the preference value programatically.

The current implementation leaves the hidden property in the schema. The property would be better hidden if it were not in the schema, as it would not then appear in auto-complete. A disadvantage of removing hidden properties from the schema is that if the preference has been set programatically then the user will see it with squiggly underlining.

If we decide it is okay that hidden properties cannot be changed programatically (we have no use case) then it should be removed from the schema.

@westbury
Copy link
Contributor Author

@vince-fugnitto On thinking about it further, it is fine to remove completely from the schema when the 'hidden' flag is set. I have pushed an updated commit that does this. Now the user cannot see any indication that the preference exists. Even if they add a value to a preference json file, ignoring the squiggly underline, the value will not be picked up and code fetching the preference value will get the default value.

Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
@westbury
Copy link
Contributor Author

@vince-fugnitto I was wondering if you will have time to look at this. We do still very much need some way of overriding preferences.

@vince-fugnitto
Copy link
Member

@vince-fugnitto I was wondering if you will have time to look at this. We do still very much need some way of overriding preferences.

@westbury I'm a bit busy at the moment with internal priorities but I will try and dedicate some time to review likely the end of next week (there are also other pull-requests to review in my backlog). It may be good to add additional reviewers as the ability to modify preferences is a feature that all downstream applications will benefit from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants