Confirm whitespace stripping #69
Conversation
This commit adds the DetectWhitespace function that used search() to determine whether or not any whitespace exists in the file. This will be used to enable a confirmation dialog.
This commit adds functionality to ask the user for confirmation to strip whitespace when they save a file (if that feature is enabled). This is enabled by default but is controlled by the strip_whitespace_confirm variable. Documentation to follow.
|
Hey, thanks for the PR! I've actually considered adding something like this. For the confirmation, I think rather than prompting for input, a more idiomatic solution would be to use :command-bang. Then you could simply make the default behavior of invoking the strip command print an error message (similar to I'm a little hesitant about adding a pre-strip whitespace check. Doing this would mean processing the buffer twice when stripping happens, and as it's currently implemented the buffer is always processed at least once, even if the strip confirmation is declined. It'd also be interesting to see what effect this has on large files. Adding additional latency is a concern since this plugin already experiences occasional slowdowns when highlighting larger files. |
|
I like the idea of using the more idiomatic :command-bang approach, however, I don't know how to 'inherit' (for lack of a better word) the 'banged' status of the :w call in the BufWritePre autocmd here. I naively tried something like The way I envision using this is not explicitly calling I understand your concerns related to the pre-strip check; I had the same thoughts. I guess it's not strictly necessary and the user could be prompted to confirm regardless of whether or not there exists whitespace; I like this solution less, but it should be faster. I've not done any testing regarding the overhead required to do the extra search, I'm sure it can be noticeable for large files. Once again, I'm open to suggestions and thanks for considering my PR! |
|
For what it's worth, the pre-strip search can be significantly faster than the actual strip action. The search seems to return as soon as a match is found whereas the strip must traverse the entire file looking for possible whitespace to strip. E.g., in a very large file, the pre-strip search takes about as long as the actual strip if the cursor is at the beginning of the file and the first whitespace is at the last line, however, if the the whitespace occurs on the second line of the file (cursor still at top of the file), the pre-strip search returns almost immediately, but the actual strip still takes the same amount of time as it must check the whole file. There are myriad use cases to consider here, but I'd say that the pre-strip search introduces relatively little overhead in the majority of them. |
Import changes from ntpeters#69 and close ntpeters#70
|
Hey, so I got around to looking into this again, and you should be able to achieve this with a combination of :command-bang (as noted above) and v:cmdbang. That way you can detect both if a user executes the command directory with a bang, or if the command is being executed indirectly via a write command with a bang. The only other thing I think is that this feature should be disabled by default. While not ideal to require opt-in for a feature that's meant to protect the user, I don't think the performance overhead of doing an extra buffer scan should be imposed on everyone out-of-the-box. This is especially true when some users already experience performance issues on slower platforms. After these last two items, I think this PR is good to go! |
Based on PR ntpeters#69 by @advocateddrummer Also make sure that EOF stripping of empty lines is only done when the EOF is in the given range.
* Allow per-buffer toggling to override global Fixes #98 * Respect global choice in current detection * Refactor for readability/maintanability Also addresses issues raised in #98. Reorder code, organise in sections. - Simplify utility functions: only keep InitVariable, remove its :exe - Whitespace patterns and highlighting mechanisms defined once - Prefs current_line_whitespace_disabled_* can be set only when loading the plugin (*), as this allows a lot of simplification. Clarify variables use/responsability. - Set buffer-local settings b:better_whitespace_enabled and b:strip_whitespace_on_save *only* when Enable/Disable/Toggle is called, or when the determination based on the filetype can be made. - Change SetupAutoCommands to be the only function to be called by: a) checking the buffer-local setting b) taking responsability for clearing the highlighting * Update docs to mirror refactor, fix #91 collisions * Fix #92: Correctly re-init on colorscheme change Bug was caused by snyIDattr() returning a string, empty when the highlight group is undefined, rather than an int with -1 for undefined. * Add mechanism for confirming whitespace stripping Based on PR #69 by @advocateddrummer Also make sure that EOF stripping of empty lines is only done when the EOF is in the given range. * Add option to only strip whitespace on modified lines Diff-based technique to close #38 * Add option to disable stripping-on-save on large files Using the number of lines in the file, based on PR #97 by @kurikomoe Also fix docs. * Adjust documentations * Check if file exists instead of file name non-emtpy Files that were created in vim printed errors on first save. E.g.: :tabe new_file :w * Fix typo in doc/better-whitespace.txt Co-Authored-By: Cimbali <[email protected]> * Fix whitespace search to also match current line Co-Authored-By: Cimbali <[email protected]> * Correct missing sentence in doc * Clarify ShouldStripWhitespace role, call diff less Use &modified and &modifiable to skip diff and disable strippping (by default) whitespace on save. * Fix incorrect diff for last line of file Appending "\n" to joined buffer lines allows to diff until EOF. Co-Authored-By: Nate Peterson <[email protected]> * Make max lines more explicit and default to 1000 * Make a command to manually strip lines on changed lines * Remove unused bangs from commands Bangs are only used of `:w` for now * Show errors to user, except E486: Pattern not found That error is the only legitimate one, but is already ignored with the e flag on :s, see :help s_e * Add error for &noro, respect range for changed lines Make commands aware of &readonly and show proper errors. Also improve StripWhitespaceOnChangedLines to take into account the range it is passed. The range to defaults to % which means no different behaviour. With any other range, only perform the stripping on the intersection of the changed lines and that range. * Add a deprecation warning about in the doc * Add error number so people can look the error up The explanation says (emphasis mine): You tried to execute a command that is neither an Ex command nor *a user-defined command*.
|
Support for this added in #100 |
|
FWIW, this got merged with the confirmation prompt defaulted to ON instead of to OFF. I use an auto-save plugin (https://github.com/907th/vim-auto-save), and I don't know who's responsible for the interaction, but however either this or that is implemented, the prompt being on totally freezes vim until it's cancelled or answered, but the prompt is not shown when autosaving occurs. This resulted in my regular development being severely hampered as vim hung repeatedly for no visible reason. It turns out vim is waiting for me to respond to this prompt, but I don't see it as long as I'm using the autosave plugin, too. For whatever reason, this particular interaction was not picked up by vim's profiling tooling, so I had to do a binary search of disabling/enabling my plugins to identify that without auto-save, I was starting to get this prompt that I didn't remember ever having every time I manually saved. I recognize that this prompt may be useful for some people, but even besides the weird interaction with auto-save, if I go back to manually saving, this is an un-asked-for and undesired interruption/distraction (another message I have to read and button I have to push) every time I save a file. The docs in this repo don't mention or feature the confirmation dialog, so this behavior will be unexpected even for new users. I've disabled it now in my .vimrc. Possibly most other users who care have already done so, or will do so shortly enough that changing the default now isn't necessary for the sake of existing users. I guess my only concrete ask is that either the docs/examples get updated, or that the default is off, so that behavior/expectations come back into alignment for new users. |
|
@toejough, sorry for the trouble you ran into! Yeah, as you've discovered the confirmation is on by default, and is used when I can definitely see this being immensely frustrating, and I may change the default due to this unfortunate behavior. It looks like the issue is that terminal Vim does not display confirmation prompts that are triggered by commands run with I also have another branch out that I'm playing with which disables confirmation by default, but as a compromise will honor |
|
Ah, I missed it somehow in the README, apologies for missing that documentation. I think it could be called out more clearly with a screenshot, but ¯\_(ツ)_/¯ I have already disabled it as you suggested, and my specific workflow is back working as I want it. Thanks for the response and investigation! Thanks for the plugin generally, as well. I try to keep my plugin set as tidy as I can, and this one has repeatedly survived my culls - it's great! |
Good afternoon @ntpeters!
I found this plugin and I like it quite a lot; I had been using a different, more convoluted, plugin for quite a while and when I switched to nvim, I began to re-evaluate my plugin usage.
The one think that I liked better about the other plugin is that is asked for confirmation before deleting whitespace during a save. I have added this functionality to your script. I don't know much about vimscript so this may be pretty bad code. But it works and I'm willing to put some effort into improving the implementation if you're willing to merge this functionality into your master version. I have also updated the documentation to reflect my changes and fixed some typos.
I'm open to feedback if you're open to the pull request!
Cheers!