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

Use file service in preference provider initialization #9362

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Apr 15, 2021

What it does

Fixes #8993 (partially) by ensuring that file-reading preference providers read the contents of their assigned files before declaring themselves ready. Ready state propagates through the scope providers to the PreferenceService, so that when the PreferenceService.ready promise resolves, all of its delegated providers will have read the contents of the files assigned to them. This still means that a .get call in a @postConstruct method that does not wait for .ready promise can get bad data, but the .ready is now reliable.

This PR doesn't go as far as the functionality suggested in #7426 because it still leaves the monaco text model creation logic in the @postConstruct method, but it moves closer.

How to test

  1. I've written a test that shows that in the tick after ready resolves, the expected preference values are available.
  2. Add a preference to your user-scoped package.json that differs from the default. E.g. "editor.fontSize" : 24, and make sure there is no conflicting specification in workspace or folder scopes of the workspace you intend to open.
  3. In a contribution with a synchronous @postConstruct method that injects the PreferenceService (e.g. packages/editor/src/browser/editor-navigation-contribution.ts), add the following lines to the .init() method:
console.log('SENTINEL FOR THE VALUE IMMEDIATELY', this.preferenceService.get('editor.fontSize'));
this.preferenceService.ready.then(() => console.log('SENTINEL FOR THE VALUE WHEN READY RESOLVES', this.preferenceService.get('editor.fontSize')));
setTimeout(() => console.log('SENTINEL FOR THE VALUE AFTER FIVE SECONDS', this.preferenceService.get('editor.fontSize')), 5000);
  1. Build and start the application.
  2. The VALUE IMMEDIATELY log will probably have the default (14, in this case); the other two should both show the configured value (20, e.g.)

Review checklist

Reminder for reviewers

Signed-off-by: Colin Grant colin.grant@ericsson.com

Sorry, something went wrong.

@colin-grant-work colin-grant-work added preferences issues related to preferences proposal feature proposals (potential future features) labels Apr 15, 2021
@colin-grant-work
Copy link
Contributor Author

I have a version of this patch that includes the lazy initialization envisioned in #7426, which I can bring here if desired, or it can be separate work.

@colin-grant-work
Copy link
Contributor Author

@kittaakos, @tsmaeder, y'all have thought a bit about this. What do you think of this approach?

@tsmaeder
Copy link
Contributor

@colin-grant-work I think it would work for the @PostConstruct case I've outlined in the issue. The important thing is to read values before we resolve the ready promise.

@colin-grant-work
Copy link
Contributor Author

@tsmaeder, are you OK with this even without changing the event emission behavior? I agree in principle that no events should be emitted until after the first read, but that would be a significant change in the behavior of the service, so I'd rather not make the change here; if there's interest in #9367, we can push ahead with a reorganization that makes sure that all framework code is aligned with the new behavior, at least.

@colin-grant-work
Copy link
Contributor Author

@tsmaeder, any thoughts?

@colin-grant-work colin-grant-work force-pushed the feature/initialize-preferences-with-file-read branch from e1c47e0 to c6f1e78 Compare May 26, 2021 15:15
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 changes work as expected for the following use-cases:

setting default application preference (through package.json):

Screen Shot 2021-05-26 at 12 05 44 PM

setting non-default preference (under settings.json):

Screen Shot 2021-05-26 at 12 12 09 PM

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.

My only minor comments are the naming present in the spec file to align with the majority of the rest of the framework.

@colin-grant-work colin-grant-work force-pushed the feature/initialize-preferences-with-file-read branch from c6f1e78 to c5834ea Compare May 26, 2021 18:22
@colin-grant-work
Copy link
Contributor Author

My only minor comments are the naming present in the spec file to align with the majority of the rest of the framework.

Good point. Done. 👍

Signed-off-by: Colin Grant <colin.grant@ericsson.com>
@colin-grant-work colin-grant-work force-pushed the feature/initialize-preferences-with-file-read branch from c5834ea to 0a528c2 Compare May 26, 2021 20:06
@colin-grant-work colin-grant-work merged commit eafb18e into eclipse-theia:master May 27, 2021
@colin-grant-work colin-grant-work deleted the feature/initialize-preferences-with-file-read branch May 27, 2021 15:25
@vince-fugnitto vince-fugnitto added this to the 1.14.0 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences proposal feature proposals (potential future features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User scoped preferences are incorrectly read from postConstruct method
3 participants