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

Lazily Create Monaco Models in Resource Preference Providers #7426

Open
tsmaeder opened this issue Mar 26, 2020 · 4 comments
Open

Lazily Create Monaco Models in Resource Preference Providers #7426

tsmaeder opened this issue Mar 26, 2020 · 4 comments
Labels
enhancement issues that are enhancements to current functionality - nice to haves help wanted issues meant to be picked up, require help monaco issues related to monaco preferences issues related to preferences

Comments

@tsmaeder
Copy link
Contributor

For a while now, AbstractResourcePreferenceProvider has used monaco editor models to manage the contents of settings files. This creates a circular dependency, because the editor model waits for preferences to be ready and preferences can't be read before the editor model is created. While this seems to work, I think it would be better if we would apply the following changes:

  1. Read the file contents directly from the file initially. Only signal "ready" once the file contents have been read
  2. Create a monaco model on demand, that is when opening an editor or when setting a preference programatically. Once we have the model, we can keep it just like now.

Opening an editor will create a model, which will wait for preferences to be ready. I also would setting a preference before the preferences provider is ready to be a programming error to be rejected.

@tsmaeder tsmaeder added enhancement issues that are enhancements to current functionality - nice to haves preferences issues related to preferences labels Mar 26, 2020
@akosyakov
Copy link
Member

We should check though that's proper documents settings are applied on preference save:

const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions();
Editor preferences are used to configure them for monaco models.

@akosyakov akosyakov added help wanted issues meant to be picked up, require help monaco issues related to monaco labels Mar 26, 2020
@tsmaeder
Copy link
Contributor Author

Yes, we would continue using models for all changes to the preferences, be it programmatically or by an editor. A save of an unchanged file should be a no-op, though (IMO).

@tsmaeder
Copy link
Contributor Author

tsmaeder commented May 31, 2021

@colin-grant-work do you think we can close this one now? I don't remember all the contents of your PR anymore. If not, what would be left to do?

@colin-grant-work
Copy link
Contributor

@tsmaeder, if you're happy, I'm happy closing it. Technically, the PR didn't do full lazy loading of the Monaco model but just introduced an additional step in the @postConstruct to read the preferences using the file system - the model is then still created as part of the init() method. It's pretty easy to do full lazy loading on top of that work if you view that as desirable, but to me, getting the load done before ready was the biggest issue, and that's taken care of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves help wanted issues meant to be picked up, require help monaco issues related to monaco preferences issues related to preferences
Projects
None yet
Development

No branches or pull requests

3 participants