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

[preferences] fix indentation when updating preferences through the tree #6736

Merged
merged 1 commit into from
Dec 20, 2019

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes #5052

Fixes the inconsistent formatting when attempting to update/set preferences using the preferences-tree widget. Since the formatting options passed to jsoncparser were hardcoded, the formatting would be inconsistent with whichever formatting the editor currently has:

const formattingOptions = { tabSize: 3, insertSpaces: true, eol: '' };

In order to fix the issue, the following method detectIndentation was implemented which attempts to determine the tabSize and indentType (insertSpaces) directly from the TextEditorDocument of the settings.json file in question. This is done in order to get a more accurate representation of the actual formatting present in the file.

The following is the before and after when adding a new preference terminal.enableCopy through the preferences-tree:

Before (using tabSize=8) After (using tabSize=8)
Screen Shot 2019-12-11 at 6 42 54 PM Screen Shot 2019-12-11 at 6 40 30 PM

How to test

  1. attempt to add preferences through the preferences-tree widget
  2. update the tabSize using the statusbar and format the document accordingly (repeat step 1)
  3. update the indentType (spaces or tabs) using the statusbar, format the document accordingly (repeat step 1)
  4. repeat step 1 using both user and workspace files

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added bug bugs found in the application preferences issues related to preferences labels Dec 11, 2019
@vince-fugnitto vince-fugnitto self-assigned this Dec 11, 2019
@lmcbout
Copy link
Contributor

lmcbout commented Dec 13, 2019

Test the format on Ubuntu 16.04 and Chrome: works fine

@vince-fugnitto vince-fugnitto force-pushed the vf/GH-5052 branch 2 times, most recently from 3009c48 to a3f3115 Compare December 16, 2019 16:28
@vince-fugnitto
Copy link
Member Author

@akosyakov I noticed a potential issue with the preferences for languages. During testing, I updated the global preference editor.insertSpaces to false, but it did not look to apply to the JSON language when getting the preference. Is this by design?

I had to do the following as a workaround:

"[json]": {
    "editor.insertSpaces": false
}

Also, not sure if we should use [json] or [jsonc] when checking for preferences.

@vince-fugnitto vince-fugnitto force-pushed the vf/GH-5052 branch 3 times, most recently from 79e336d to 575670d Compare December 16, 2019 17:05
@akosyakov
Copy link
Member

@vince-fugnitto I think jsonc should be proper for preferences.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code-wise it looks good to me

Fixes #5052

- fixes the indentation when updating/setting a preference using the
`preferences-tree` widget.
- the current implementation had a hardcoded value of `tabSize`=3 and `insertSpaces`=true which
meant that if the `settings.json` file had different formatting, it would insert incorrectly.
- added a new method `getFormattingOptions` which gets the preference options for tabSize and indentation type.
- fixed typings from the `preference-service`. If a default value is provided, the preference should return that
value if the preference initially returns `undefined`.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto
Copy link
Member Author

@lmcbout if you get a chance, can you please re-test the pull request?
The overall strategy has been changed and simplified.

@lmcbout
Copy link
Contributor

lmcbout commented Dec 19, 2019

Tested with @vince-fugnitto not perfect, but OK

@vince-fugnitto
Copy link
Member Author

Tested with @vince-fugnitto not perfect, but OK

@lmcbout I completely understand, the pull request becomes best effort when the settings document is not properly formatted or is formatted inconsistently (different tabSize and indentation throughout the document).

When the document is properly formatted, inserting preferences from the preferences-tree is more consistent. Overall, the pull request improves upon the original implementation which used hard coded values and would always format inconsistently.

Perhaps we can improve the pull request further in the future, but I am unsure what an appropriate strategy would be when working with un-formatted documents.

@vince-fugnitto vince-fugnitto merged commit 2b13d55 into master Dec 20, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-5052 branch December 20, 2019 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application preferences issues related to preferences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[preferences] inconsistent formatting when adding a preference through the tree
3 participants