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

Defaults not always correct #3

Closed
SamVerschueren opened this issue Feb 8, 2016 · 9 comments
Closed

Defaults not always correct #3

SamVerschueren opened this issue Feb 8, 2016 · 9 comments
Labels

Comments

@SamVerschueren
Copy link
Collaborator

this block tries to use the defaults of the texteditor and in my case, in most circumstances, it works. But not always. Let's assume we have this .editorconfig file.

root = true

[{package.json,*.yml}]
indent_style = space

Expected behaviour

If my VS Code settings of insertSpaces is set to auto, it should auto-detect the the indentation type (spaces or tabs). This means that if I open index.js, (which uses tabs) it should detect the tabs and use that is indent style.

Current behaviour

  1. Open a project and open package.json as the FIRST file (this will set spaces as default indent type)
  2. Open index.js (which uses tabs) and it will use spaces as current indent type

I will create an issue in the VSCode issue tracker and link this one. They have to refactor the API in order for us to support automatically detected defaults by VS Code.

@SamVerschueren
Copy link
Collaborator Author

I found the solution. Will do a PR tomorrow :).

@jednano
Copy link
Member

jednano commented Feb 8, 2016

Excellent! I look forward to seeing the solution. Thanks for your dedication to this extension!

@SamVerschueren
Copy link
Collaborator Author

Damn, I though I had it but unfortunately I was wrong. This is what I thought was working.

Instead of creating an entirely new options object for TextEditor.options, I only set the option that changed.

if (editorConfig.indent_style) {
    textEditor.options.insertSpaces = editorConfig.indent_style !== 'tab';
}

if (editorConfig.indent_size || editorConfig.tab_size) {
    textEditor.options.tabSize = editorConfig.tab_width || editorConfig.indent_size;
}

I thought that when indent_style was not overwritten, the editor would calculate it for me, but unfortunately I was wrong. It seems that when you do it like above, the settings are overwritten by VS Code because textEditor.options still has a reference to the internal options object. It looks like that is how it determines to recalculate the settings based on the users settings.

  • Is the options object still referring to the initial options object?
    -> YES: No plugin changed the settings so recalculate them based on the user settings
    -> NO: Do nothing as someone altered the object

This is the issue I created in the VSCode repo. microsoft/vscode#2797

@jednano
Copy link
Member

jednano commented Feb 9, 2016

@SamVerschueren if the vscode logic is working as you theorize, what if we use Object.assign to merge new settings w/ user settings. Do you think that would solve the issue?

@SamVerschueren
Copy link
Collaborator Author

No, because when the usersettings are set to auto, the value depends on the file being opened. If for instance you open a file with tabs, and the insertSpaces setting is set to auto, it will auto-detect that it should insert tabs.

But we as a plugin are not able to retrieve that detected value. Actually we can, but our code is invoked before VS Code detects the settings.

So this is the flow without our plugin

  1. Open a file
  2. VS Code looks at the textEditor.options object, it wasn't altered by the plugin so use the values from the settings file. If the value is auto, auto-detect the values.

This is the flow with our plugin

  1. Open a file
  2. We change textEditor.options depending on the values in .editorconfig, but at this point we don't have the auto-detected values
  3. VS Code looks at the textEditor.options object, it was altered by the plugin so it stops looking into the user settings.

Does this make things clear :)? It's difficult to explain :p.

@jednano
Copy link
Member

jednano commented Feb 9, 2016

Thanks for the explanation. If I understand correctly, you're saying that the moment we've tampered with the tabSize setting – not even touching the insertSpaces setting – vscode bails out of the auto-detection, because any modification to textEditor.options is enough for vscode to bail out.

After slowly reading your comment, it seems to me that the problem only presents itself when you want to apply "some" settings, but not necessarily all of them. In your comment's example, you want index.js to apply the indent_style of tab, but since the .editorconfig file doesn't have an indent_size setting for index.js, then you want vscode to go ahead and fall back to your user settings, in which auto would be the indentSize. If you were to remove indent_style from your .editorconfig file then vscode would work fine, because we wouldn't need to tamper with textEditor.options at all in that case.

If, however, the future allowed us to support the end_of_line setting with an exposed textEditor.options.endOfLine setting, even THAT setting would be screwing with our auto indentation, because it would require us to manipulate, again, the textEditor.options object.

I get it now, I think. You're right that it's hard to explain. I can't just quickly read your explanation or I'll be confused. I guess we just track microsoft/vscode#2797 until we can move forward with this.

@SamVerschueren
Copy link
Collaborator Author

if I understand correctly, you're saying that the moment we've tampered with the tabSize setting – not even touching the insertSpaces setting – vscode bails out of the auto-detection

It only bails out when you apply an entirely new object. But only tampering one of the two like this

textEditor.options.insertSpaces = true;

isn't working. It doesn't take that setting. So what I understand from this is that VS Code looks at the options object reference to determine if it should apply user settings or not.

After slowly reading your comment, it seems to me that the problem only presents itself when you want to apply "some" settings, but not necessarily all of them.

Exactly!

@jednano
Copy link
Member

jednano commented Feb 9, 2016

That's very odd that modifying one setting doesn't work. They need to fix that!

@SamVerschueren
Copy link
Collaborator Author

I think that is how they determine if the object was modified. Something (pseudo-codeish) like this

const _options = {
    insertSpaces: false,
    tabSize: 4
};

const initialReference = _options;

Object.defineProperty(textEditor, 'options', {
    get: () => _options,
    set: (newOptions) => {
        _options = newOptions;
    }
});

// ...
if(_options === initialReference) {
    // Object was not modified, parse the settings and auto-detect if value is "auto"
} else {
    // Do nothing because the object was modified by a plugin
}

I'm not sure though because I can't find the code that is responsible for this. It's just how I think it is implemented depending on the behaviour I see.

A solution would be to be able to pass in undefined value.

textEditor.option = { insertSpaces: true };

Because tabSize is undefined, it should parse the settings from the user. It would make much morse sense.

Or off course fix the behaviour to be able to modify one setting directly.

textEditor.option.insertSpaces = true;

jednano pushed a commit that referenced this issue Feb 12, 2016
add support for defaults - fixes #3
@jednano jednano added the bug label Feb 12, 2016
@jednano jednano added this to the EditorConfig 1.0 milestone Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants