jtokoph / auto-detect-indentation

Auto detect indentation of files in Atom
MIT License
39 stars 6 forks source link

Add menu to select indentation level for an editor #11

Closed rogerhub closed 8 years ago

rogerhub commented 8 years ago

This might be out of scope for this plugin, but I find it useful to be able to change the editor's indentation manually. Autodetection doesn't always get it correct, and there's currently no other way to change the indentation for a new file.

This patch adds both the status bar element (the one that says "2 Spaces") and the dropdown menu (hotkey Ctrl+Shift+I).

screenshot 2016-03-04 10 43 16

jtokoph commented 8 years ago

Any thoughts on the removal of the onSave handler? I'm assuming you don't want it to auto-detect again if you've manually changed the settings. If so, what if there is flag to disable the future detection after it's been manually set?

rogerhub commented 8 years ago

I was going to ask you about that. What was the reasoning behind re-detecting indentation when the file is saved? Is it so this workflow will work correctly?

I took it out because it was resetting the manually-overridden indentation. I can add in a flag to check though.

rogerhub commented 8 years ago

Also, it feels impure to set properties on editor. Is there a better way to accomplish the same effect? I had to set a property on editor to prevent another issue (when the indent length is changed, the buffer retokenizes, which triggers auto-detection of indentatation).

jtokoph commented 8 years ago

Yeah that was the main flow I was concerned about.

I'm not sure what the best practice is. I can't really find any other packages that store something similar. One option is to keep a cache internally, keyed by editor.id.toString(). I'll dig a little more.

jtokoph commented 8 years ago

Also looks like the keybind conflicts with opening dev tools on linux: https://github.com/atom/atom/blob/master/keymaps/linux.cson

jtokoph commented 8 years ago

An option is to not give a default keybind and allow users to specify one in their keymap if they want.

rogerhub commented 8 years ago

Ah, okay. I'll remove the keybinding as well.

rogerhub commented 8 years ago

Okay I took out the keymaps and restored the onDidSave handler. I also made the indentationTypes configurable.

jtokoph commented 8 years ago

Just had a suggestion from someone in atom Slack.

You could use a feature called WeakMaps.

It lets you key things based on an object.

var syntaxLoaded = new WeakMap()
syntaxLoaded.set(editor, true);
console.log(sytaxLoaded.get(editor)); // true
rogerhub commented 8 years ago

I got rid of both of the custom properties. For one of them, I just let the handler remove itself, because it only needs to run once. For the other, I used a WeakSet. I also did some refactoring so that the indentation code is centralized. Let me know what you think.

jtokoph commented 8 years ago

Merged! Thanks so much for the contribution. I added a note to the readme.

I also added one more commit with a couple tweaks: https://github.com/jtokoph/auto-detect-indentation/commit/8adc513c0d308127664b1ed45650c524553d8383

rogerhub commented 8 years ago

Thanks for the responsiveness! The namespace change looks good, and should have been done in the first place. I was mostly just following the grammar selector plugin as a guide, which is where that name came from.