Add command to toggle CSV column aligning#45
Add command to toggle CSV column aligning#45mkruselj wants to merge 1 commit intomechatroner:masterfrom
Conversation
mechatroner
left a comment
mechatroner
left a comment
There was a problem hiding this comment.
Thanks for your contribution! The feature itself sounds nice, but there are some minor issues with the implementation.
| else: | ||
| self.view.run_command('align') | ||
|
|
||
| def is_table_aligned(self, delim, policy): |
There was a problem hiding this comment.
I would prefer to have a global dictionary variable is_aligned_map which maps filenames to their current alignment status. By default, it should be "false" i.e. not aligned.
There was a problem hiding this comment.
Mmmm wouldn't this potentially create problems if you open the same file in multiple views and set the alignment up differently?
I prefer a slight runtime cost over increasing memory requirement, and this method is sure to work even if the same file is opened in multiple views with different state of alignment...
There was a problem hiding this comment.
The runtime check has some issues, e.g. user can edit the file - add/remove whitespaces and it would run the same op again, for different files initial behavior would be different when you map it to the command, since there is no feedback on what to expect. First command invocation should always align. You have a good point about multiple view, I'm actually not sure if it aligns in a single view only, I would appreciate if you check, but if it indeed the case then we have to store the variable in the view state (if there is such a thing) or use view id or something similar. I don't like heuristics and guessing when it is possible to avoid them with explicit state-based logic
| { "command": "set_table_name", "caption": "Set table name for RBQL" }, | ||
| { "command": "align", "caption": "Align (with trailing spaces)" }, | ||
| { "command": "shrink", "caption": "Shrink (trim spaces from fields)" }, | ||
| { "command": "toggle_csv_alignment", "caption": "Align CSV columns with spaces", "checked": true }, |
There was a problem hiding this comment.
what does "checked": true mean?
There was a problem hiding this comment.
This enables Sublime Text to run the is_checked() method, which then shows or doesn't show the checkmark next to the action name in the context menu.
There was a problem hiding this comment.
Let's just remove the new command from the context menu. Instead, add documentation on how to add hotkey mapping for it, those who need it would do so.
| { "command": "disable", "caption": "Disable" }, | ||
| { "command": "run_query", "caption": "Run RBQL query (F5)" }, | ||
| { "command": "set_table_name", "caption": "Set table name for RBQL" }, | ||
| { "command": "align", "caption": "Align (with trailing spaces)" }, |
There was a problem hiding this comment.
Let's keep the original commands.
There was a problem hiding this comment.
Kind of doesn't make sense when this one is arguably better? I consider it information overload, you have three actions that are all tied to the exact same thing, and two of them are basically redundant...
There was a problem hiding this comment.
To your point, I would actually keep the 2 original commands and remove the toggle command, it is nice to have it as a button (but there is no way to customize UI in Sublime AFAIK) or as a command.
There was a problem hiding this comment.
I really don't see why it's such a problem to have a neat single action instead of two clunky ones... I may be biased but this single action is simply better UX regardless if it's on a button in the GUI, if it's a keybind, or if it's a menu entry...
There was a problem hiding this comment.
Here is my thinking: It does make sense to have a single keybinding to the toggle command, because it is harder to remember 2 keybindings than one. User mental memory is important. So it is fine to make the command less precise, but the upside is we save the user from the headache. Same goes for the UI button in the front screen - the screen area is limited so we only can afford to have 1 less precise button. Totally different situation with the context menu, it only shown in rare situations and we want to be precise there, I don't mind to have 2 buttons in the context menu and IMO it is more convenient to have it this way. WDYT?
There was a problem hiding this comment.
I am not sure if it's really more convenient to have two separate context menu entries that both deal with the exact same thing. This is the best possible case for checkmarked single entries. Less cognitive load (two separate context menu entries have different wording, here you have one single wording and a checkmark to tell you it's on - easy!)
😅
| [ | ||
| {"keys": ["f5"], "command": "run_query", "context": [{"key": "setting.rainbow_mode", "operator": "equal", "operand": true}]} | ||
| {"keys": ["f5"], "command": "run_query", "context": [{"key": "setting.rainbow_mode", "operator": "equal", "operand": true}]}, | ||
| {"keys": ["ctrl+alt+a"], "command": "toggle_csv_alignment"} |
There was a problem hiding this comment.
Let's avoid adding additional key mappings, but instead could you please add a readme instructions how users can create this mapping in their config.
There was a problem hiding this comment.
Just to give you a perspective on why I really wanted to have a default keybinding here.
As a user, the one thing that annoys me with a lot of Sublime Text extensions is that they don't come with some keybindings to get you started, instead you have to dig around settings files and set up the keybinding manually by writing a JSON incantation. So I thought to actually improve UX for users here, they can just start using the thing straight up, and this align/compact is (after syntax coloring CSVs) the most important feature of this plugin, as far as I'm concerned. It makes things so much easier when editing big CSVs. So being able to do that without any additional setup would have been a huge benefit in my book...
| { "command": "disable", "caption": "Disable" }, | ||
| { "command": "run_query", "caption": "Run RBQL query (F5)" }, | ||
| { "command": "set_table_name", "caption": "Set table name for RBQL" }, | ||
| { "command": "align", "caption": "Align (with trailing spaces)" }, |
There was a problem hiding this comment.
To your point, I would actually keep the 2 original commands and remove the toggle command, it is nice to have it as a button (but there is no way to customize UI in Sublime AFAIK) or as a command.
| { "command": "set_table_name", "caption": "Set table name for RBQL" }, | ||
| { "command": "align", "caption": "Align (with trailing spaces)" }, | ||
| { "command": "shrink", "caption": "Shrink (trim spaces from fields)" }, | ||
| { "command": "toggle_csv_alignment", "caption": "Align CSV columns with spaces", "checked": true }, |
There was a problem hiding this comment.
Let's just remove the new command from the context menu. Instead, add documentation on how to add hotkey mapping for it, those who need it would do so.
| else: | ||
| self.view.run_command('align') | ||
|
|
||
| def is_table_aligned(self, delim, policy): |
There was a problem hiding this comment.
The runtime check has some issues, e.g. user can edit the file - add/remove whitespaces and it would run the same op again, for different files initial behavior would be different when you map it to the command, since there is no feedback on what to expect. First command invocation should always align. You have a good point about multiple view, I'm actually not sure if it aligns in a single view only, I would appreciate if you check, but if it indeed the case then we have to store the variable in the view state (if there is such a thing) or use view id or something similar. I don't like heuristics and guessing when it is possible to avoid them with explicit state-based logic
2 participants
I was missing a simple command to toggle between aligning the columns then shrinking them back. So here it is! UX improvement is considerable.
I took liberty in replacing the individual align/shrink commands from the context menu with this new command that also shows the state with a checkmark. Also added a keyboard shortcut that is not used by Sublime Text by default (Ctrl+Alt+A).
Thank you for your consideration!