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

Read TextEditor options from active editor #444

Merged
merged 1 commit into from
Jul 14, 2016

Conversation

rebornix
Copy link
Member

Yay! We love PRs! 🎊

Please include a description of your change & check your PR against this list, thanks:

  • Commit message has a short title & issue references
  • Commits are squashed
  • It builds and tests pass (e.g gulp tslint)

We were reading TextEditor options from configuration but there are some drawbacks:

  1. We didn't monitor the configuration change (not sure if there is any event exposed)
  2. VS Code puts a copy of the text editor related configuration in each Text Editor object and there is no bidirectional between them. If people change the configuration file, every copy of text editor option will be reloaded but if we change vscode.window.activeTextEditor.options through API on the fly, it's not synced back to vscode.workspace.getConfiguration("editor"). It does make sense because vscode.workspace.getConfiguration("editor") is likely representing the state of the configuration file.

Here I changed our logic to read from vscode.window.activeTextEditor.options always because we can't make sure we are the only one that modifies it. People might install other extensions that modify the options at runtime as well. vscode.window.activeTextEditor.options is our best choice now.

BTW, I'll raise the issue to @code team about the configuration sync issue because we are adding API support for other configuration part.

@johnfn
Copy link
Member

johnfn commented Jul 14, 2016

LGTM 👍

@johnfn johnfn closed this Jul 14, 2016
@johnfn johnfn reopened this Jul 14, 2016
@johnfn johnfn merged commit b534567 into VSCodeVim:master Jul 14, 2016
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants