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

Synchronize Node.js <-> TS settings #995

Merged
merged 3 commits into from
Jun 8, 2016
Merged

Conversation

mousetraps
Copy link
Contributor

Synchronize Node.js <-> TS settings so that Node.js options are applied to Salsa. In particular this change:

  • on startup, populate Node.js options with TS -> Node.js editor settings (general, scrollbars, and tabs)
  • on Node.js editor settings changed, populate TS options
  • use TS properties as backing for Node.js formatting settings

limitations:

  • On TS options changed, will not sync back to Node.js
  • On TS + Node.js options changed, will use Node.js options

…ed to Salsa. In particular this change:

* on startup, populate Node.js options with TS -> Node.js editor settings (general, scrollbars, and tabs)
* on Node.js editor settings changed, populate TS options
* use TS properties as backing for Node.js formatting settings

limitations:
* On TS options changed, will not sync back to Node.js
* On TS + Node.js options changed, will use Node.js options
_preferences.fAutoListParams = langPrefs[0].fAutoListParams;
_preferences.fHideAdvancedAutoListMembers = langPrefs[0].fHideAdvancedAutoListMembers;
public int OnUserPreferencesChanged4(VIEWPREFERENCES3[] pViewPrefs, LANGPREFERENCES3[] pLangPrefs, FONTCOLORPREFERENCES2[] pColorPrefs) {
IVsTextManager4 textMgr = (IVsTextManager4)NodejsPackage.Instance.GetService(typeof(SVsTextManager));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can textMgr ever be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't null check anywhere, and I wouldn't expect it to ever be null (except maybe inside of some of the mock test cases?)

@mjbvz mjbvz added this to the 1.2 Beta milestone Jun 8, 2016
@mjbvz
Copy link
Contributor

mjbvz commented Jun 8, 2016

Look good overall. Let's get the CI passing and make sure to test this on a machine without the VS sdk installed and then merge this in for the 1.2 beta.

@mjbvz mjbvz merged commit bd83e28 into microsoft:master Jun 8, 2016
@mousetraps mousetraps mentioned this pull request Jun 8, 2016
23 tasks
mjbvz added a commit to mjbvz/nodejstools that referenced this pull request Jun 10, 2016
Issue microsoft#1014

**Bug**
Crash in `SmartIndent.cs` in the interactive window. I believe that the root cause is microsoft#995, which changed how we store our settings.

**Fix**
Switch to using the NodeJs language preferences from typescript instead. This ensures we are using the same logic to get the language settings as we use in other places.

closes microsoft#1014
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.

3 participants