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

Add validations to Monaco #11257

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #11241 by removing the buggy ValidatedPreferenceProxy implementation in favor of adding validations for a few key preferences to Monaco's own validation system for editor preferences.

How to test

  1. Open a workspace and open a text file in that workspace.
  2. In that workspace, set e.g. editor.fontSize to some non-default value.
  3. Observe that the new setting takes effect in your open file.
  4. In user scope, set the same preference to some other non-default value.
  5. Observe that that new setting does not take effect in your open file because it is overridden in a higher-priority scope.

  1. Also confirm that the testing steps for Validate Preferences Used in Editor #10607 and Ensure that old values aren't cached inappropriately #10965 still go as expected.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added preferences issues related to preferences monaco issues related to monaco labels Jun 6, 2022
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the first part of the steps to test is successful, preferences at higher scopes have precedence and apply (ex: workspace over user).

I did notice however that:

  • the browser tests fail on CI
  • there is no validation message when setting an invalid preference (even with #11025 I understood that it should happen for manual updates by the user)

@colin-grant-work
Copy link
Contributor Author

  • the browser tests fail on CI

I'll check into this.

  • there is no validation message when setting an invalid preference (even with #11025 I understood that it should happen for manual updates by the user)

This is expected: Monaco doesn't provide any indication that the user has done something wrong; it just uses a different value. With this code, the PreferenceValidatioService, which issued those warnings, is no longer in use, but I haven't removed it because I think it could be a useful way to provide feedback in the preference UI, with tweaks to provide detailed information. Have to compare it with just using AJV, though.

@msujew msujew self-requested a review June 13, 2022 15:43
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes are looking good to me 👍

  • Preferences scopes seem to use the correct precedence now
  • Monaco preferences (minimap, rendering whitespace) continues to work correctly
  • Setting a preference manually to an invalid value shows a warning and the default value is applied

@colin-grant-work colin-grant-work force-pushed the bugfix/supplement-monaco-validation branch from 8f96b3a to 2517465 Compare June 22, 2022 18:09
@colin-grant-work colin-grant-work force-pushed the bugfix/supplement-monaco-validation branch from 2517465 to bb1f3a4 Compare June 22, 2022 18:12
@colin-grant-work colin-grant-work merged commit bedc8b1 into eclipse-theia:master Jun 23, 2022
@colin-grant-work colin-grant-work deleted the bugfix/supplement-monaco-validation branch June 23, 2022 22:06
@colin-grant-work colin-grant-work added this to the 1.27.0 milestone Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor preferences ignoring scope precedence
3 participants