sindresorhus / atom-editorconfig

Helps developers maintain consistent coding styles between different editors
https://atom.io/packages/editorconfig
MIT License
812 stars 80 forks source link

Atom forgets tabtype configuration #157

Closed rumpelsepp closed 7 years ago

rumpelsepp commented 7 years ago

Atom forgets the tab type configuration when this plugin is enabled. I use the autodetect feature of atom, to detect if a buffer uses tabs or spaces for the intendation. When I change tabs, atom forgets that setting. Disabling this plugin fixes this issue, so it might be caused by atom-editorconfig.

In the attached gif I create a new line, switch tabs and then create a new line again. You see that after switching tabs spaces are used. out

Involved .editorconfig-files

None; just the plugin is enabled

Directory structure

Not relevavant I guess.

Installed packages

Built-in Atom Packages (89)
├── atom-dark-syntax@0.27.0
├── atom-dark-ui@0.52.0
├── atom-light-syntax@0.28.0
├── atom-light-ui@0.45.0
├── base16-tomorrow-dark-theme@1.3.0
├── base16-tomorrow-light-theme@1.3.0
├── one-dark-ui@1.6.2
├── one-light-ui@1.6.2
├── one-dark-syntax@1.5.0
├── one-light-syntax@1.5.0
├── solarized-dark-syntax@1.0.5
├── solarized-light-syntax@1.0.5
├── about@1.7.0
├── archive-view@0.62.0
├── autocomplete-atom-api@0.10.0
├── autocomplete-css@0.13.1
├── autocomplete-html@0.7.2
├── autocomplete-plus@2.31.4
├── autocomplete-snippets@1.11.0
├── autoflow@0.27.0
├── autosave@0.23.1
├── background-tips@0.26.1
├── bookmarks@0.42.0
├── bracket-matcher@0.82.2
├── command-palette@0.39.0
├── deprecation-cop@0.54.1
├── dev-live-reload@0.47.0
├── encoding-selector@0.22.0
├── exception-reporting@0.40.0
├── find-and-replace@0.202.2
├── fuzzy-finder@1.4.0
├── git-diff@1.1.0
├── go-to-line@0.31.0
├── grammar-selector@0.48.2
├── image-view@0.60.0
├── incompatible-packages@0.26.1
├── keybinding-resolver@0.35.0
├── line-ending-selector@0.5.0
├── link@0.31.2
├── markdown-preview@0.158.8
├── metrics@1.1.1
├── notifications@0.65.1
├── open-on-github@1.2.1
├── package-generator@1.0.1
├── settings-view@0.243.1
├── snippets@1.0.4
├── spell-check@0.68.4
├── status-bar@1.4.1
├── styleguide@0.47.2
├── symbols-view@0.113.1
├── tabs@0.103.0
├── timecop@0.33.2
├── tree-view@0.210.0
├── update-package-dependencies@0.10.0
├── welcome@0.35.1
├── whitespace@0.35.0
├── wrap-guide@0.38.2
├── language-c@0.54.0
├── language-clojure@0.22.1
├── language-coffee-script@0.48.0
├── language-csharp@0.12.1
├── language-css@0.40.1
├── language-gfm@0.88.0
├── language-git@0.15.0
├── language-go@0.43.0
├── language-html@0.46.1
├── language-hyperlink@0.16.1
├── language-java@0.24.0
├── language-javascript@0.122.0
├── language-json@0.18.3
├── language-less@0.29.6
├── language-make@0.22.2
├── language-mustache@0.13.0
├── language-objective-c@0.15.1
├── language-perl@0.37.0
├── language-php@0.37.3
├── language-property-list@0.8.0
├── language-python@0.45.1
├── language-ruby@0.70.2
├── language-ruby-on-rails@0.25.1
├── language-sass@0.57.0
├── language-shellscript@0.23.0
├── language-source@0.9.0
├── language-sql@0.25.0
├── language-text@0.7.1
├── language-todo@0.29.1
├── language-toml@0.18.1
├── language-xml@0.34.12
└── language-yaml@0.27.1

Community Packages (22) /home/stefan/.atom/packages
├── asciidoc-assistant@0.2.1
├── asciidoc-image-helper@1.0.0
├── asciidoc-preview@2.6.0
├── autocomplete-asciidoc@0.1.1
├── autocomplete-clang@0.10.0
├── autocomplete-go@1.3.0
├── autocomplete-python@1.8.12
├── cursor-history@0.7.0
├── editorconfig@2.0.5
├── file-icons@1.7.25
├── git-time-machine@1.5.4
├── go-config@1.2.4
├── go-get@2.1.1
├── godoc@1.1.1
├── gofmt@1.2.0
├── language-asciidoc@1.5.3
├── language-fish-shell@1.0.5
├── language-latex@1.0.0
├── pdf-view@0.53.0
├── todo-show@1.8.0
└── vim-mode-plus@0.66.1
florianb commented 7 years ago

Hi @rumpelsepp, please invoke EditorConfig: Show State to determine if editorconfig mangling your tab-type because of an editorconfig-setting (that's the case if f.e. the indent_style is different to auto).

Editorconfig should only touch the tab type, if indent_style is configured. If it is configured it always overrides the Atom-settings. This is expected behavior.

rumpelsepp commented 7 years ago

Since I have no editorconfig files in that project, EditorConfig: Show State always shows auto.

florianb commented 7 years ago

🤔 If it shows auto, editorconfig shouldn't change anything..

florianb commented 7 years ago

Well, i am wrong (the responsible line is https://github.com/sindresorhus/atom-editorconfig/blob/master/index.js#L136). To revert changes possibly made by editorconfig we currently explicit apply the atom-config setting for that scope.

I assume this is why your setting gets reset to something different than expected. I have to think about how this case must be treated correctly.

florianb commented 7 years ago

🗒 Reference changed to https://github.com/sindresorhus/atom-editorconfig/blob/master/index.js#L57

kirsle commented 7 years ago

@florianb when viewing a GitHub source file, press the y key to get a permalink including the commit hash you were looking at; then code references in issues always point to the same place you originally mentioned, even if the source file is updated in future commits (and even when the source file is removed or renamed!)

The permalink to that source line of code is https://github.com/sindresorhus/atom-editorconfig/blob/773d07bd230e2e91d441e3ac4cadce9025562185/index.js#L57

dmytrokyrychuk commented 7 years ago

The tab type is actually stored under editor.tabType key in config. editor.softTabs is only used when editor.tabType is set to auto and Atom fails to determine the tab type of a buffer (which happens when a buffer does not contain any leading whitespace). It is not correct to set the tab type based on editor.softTabs value only.

See how Atom v1.14.3 determines the tab type for an editor: the function and its usage.

florianb commented 7 years ago

Thanks for your comment @orgkhnargh, even though i don't know what it should tell me - or was it addressing kirsle?

Atom-Editorconfig requires the editor.tabType being set to auto anyways.

dmytrokyrychuk commented 7 years ago

@florianb I'm sorry if my comment was not clear. It was addressed to you, @florianb.

I'll try to explain. When editor.tabType is set to auto, it's necessary to examine the buffer content in order to determine whether spaces or tabs are being used (atom editors have usesSoftTabs method that does just that). Atom-Editorconfig does not examine the buffer content, it just sets the default value (editor.softTabs).

Here is a list of steps to reproduce the issue shown on @rumpelsepp's gif.

  1. Disable the atom-editorconfig package and restart Atom.
  2. Make sure editor.tabType is set to auto and editor.softTabs is set to true.
  3. Create a file with the following content:
    line with tab indent
    another line with tab indent
  4. Make sure that atom uses hard tabs for this file.
  5. Create .editorconfig file with the following content. This will prevent editorconfig from using any global configuration files, and all the settings will be implicitly set to auto.
    root = true
  6. Enable the atom-editorconfig package.
  7. See what indent style is used for the file now. Expected result: tabs (as all the other indents are made of tabs. Actual result: spaces (because the content is ignored and the tab style is set based on the editor.softTabs).
florianb commented 7 years ago

Thanks @orgkhnargh - i guess i got it now. I drafted a fix, i'd appreciate if you could take a look! However, i still have to write a test for it.

dmytrokyrychuk commented 7 years ago

@florianb looks fine to me!

The only thing I am concerned about is that it still ignores the editor.tabType value, which is not consistent with the behavior of Atom without atom-editorconfig. IMO, when a user does not specify indent_style for a file in .editorconfig, the logic of determining the indent style should fall back to what Atom uses. Currently atom-editorconfig implicitly decides that editor.tabType is set to auto, but people could have set editor.tabType to either hard or soft too. I'm not sure why would anyone do that instead of specifying indent_style in the [*] section of .editorconfig, but it is a possibility that might be reasonable to consider.

florianb commented 7 years ago

Thanks @orgkhnargh.

I in fact decided to not support Atom's tabType being set to anything else than auto because Atom seems to internally invoke updates of the editor's configuration which results in a override of the tabType settings done by this package.

So when a tabType different to auto is detected the user is warned that indent_styles may not be applied correctly. To limit the experience of unexpected behavior we could disable the treatment of indent_styles entirely as long as the tabType isn't set to auto.

dmytrokyrychuk commented 7 years ago

@florianb I understand your point, but I don't think it makes sense. The only time when the settings of an editor may be different from the ones listed in .editorconfig is after user closes (or switches off from) the settings tab and before user activates an editor (atom-editorconfig reapplies the settings on top of what Atom applied when an editor is activated).

The only way to break this I could think of is when the user changes editor.tabType somehow without switching from the active editor (e.g., they could make a tollbar button that switches tab type in the settings). In that case the tab type of the active editor will be changed by Atom. Only the active editor will be affected, because as soon as the user switches to another editor, tab type there will be reapplied by atom-editorconfig, as well as the tab type of the editor that was active during the change as soon as the user switches back to it. Even this could hopefully be fixed by reapplying the editorconfig settings on editor.tabType and editor.softTabs.

I don't really see how not setting editor.tabType to auto could interfere with atom-editorconfig. I can see that changing editor.tabType while atom-editorconfig is active may lead to issues with one (currently open) editor when user does not use settings-view for configuration. But this only means that you should warn users not to change tab type, as the process of changing is the cause of issues, not the values they change tabType to. Having tabType set to other values cannot be harmful as long as atom-editorconfig handles them correctly.

The above is the conclusion I made after I read both atom and atom-editorconfig source code. Please let me know if I am wrong.

florianb commented 7 years ago

@orgkhnargh the decision to stick with tabType being auto was made in April 2016. And one reason is that the tabType is also being applied on tokenization. The tokenization API of the text-buffer is considered as private. So i treat that part as black box which i can't affect.

So i would like to ask what this discussion is about and what you are aiming for?

Applying the editorconfig's standard to Atom has a lot caveats and some parts still rely on moving parts of the Atom environment. So if you would like to enhance the (pretty stable) tabType-functionality - feel free to play around with a fork. I would really appreciate to have more practical contributions.

dmytrokyrychuk commented 7 years ago

@florianb I am just trying to bring to attention the moving parts of the Atom environment that aren't that moving and could be handled correctly.

Please excuse me if I was too annoying :white_flag:

florianb commented 7 years ago

@orgkhnargh i guess i don't get it again. The editor.tabType is everytime applied a tokenize-event get's fired, unless it is set to auto. I don't see any way to intercept that, or to get notified of that in a way which guarantees that this package is being the last one in the event-chain which is called. Until this is solved, we can't set the softTabs without being overridden, unless the tabType is set to auto.

You brought attention to a bug in the implementation which is going to be fixed. And i am thankful for that.

But i can't see that the further discussion about the requirement of tabType being set to auto fixes anything. Because in my understanding there is nothing more to fix. But i am only a human, so if i just don't see the case of a incorrect implementation besides of this issue, please open a new issue and file a spec which points out where the current implementation fails. That way i guess the chance lowers i am able to misunderstand what you're aiming for.

florianb commented 7 years ago

@brumm i rolled the patch out for this issue. I'd appreciate if you could confirm that this issue is fixed.