helix
helix copied to clipboard
Fix for saving from insert mode
Fixes #6513, fixes #3501, addresses #1583. This somewhat changes the granularity of the undo history: but keeps the preciseness. While adding to the undo history on a save may not seem idiomatic conceptually at first: I think it's the best way to address the issue, because 1) saving a file is somewhat of a "finishing" action and 2) comparing the undo history with the current state of a file is quite a nice way to check if something has been modified and I would prefer not to mess with it.
I also added e2a877432ce7f75eba60d5dd579e5a0ab45b73e8 because I stumbled across it while tracking down this issue and thought that having a separate history entry for a raw paste before any manipulation of it would be desirable. I'd be fine rolling it back but that also propagating to history makes more sense to me.
https://github.com/helix-editor/helix/pull/2883#pullrequestreview-1029452126
While committing changes from insert mode is perhaps not idiomatic, this fixes a bug. Any shortcuts to save a file from insert mode are currently broken: they do not update the file modification indicator and Helix thinks the file has not been written and will not let you close it, see #6513.
This shouldn't terribly change how anyone not binding to :write uses Helix, and makes Helix more usable for anyone binding to :write.
Looking at the changes you only add doc.append_changes_to_history(view) into write_impl but not into write_all_impl.
Why not just stop the ability to save in insert mode? write could check you are in normal mode, and if not return an error. It would enforce best practice. https://github.com/helix-editor/helix/pull/2883#pullrequestreview-1029452126 explained, and archseer amplified this sentiment by quoting it.
I admit when I first started using Helix I bound ctrl-s to save and saved all the time in insert mode. This was a mistake and led me down the wrong path for using Helix, if it were not possible I never would have wasted my time doing it.
I think it's worth fixing this regardless of the use-case, although I'm not a huge fan of the saving-in-insert-mode use-case. Ideally we shouldn't have to think about making commit checkpoints in typeable commands like :clipboard-paste-replace - it's pretty easy to forget to append changes to history there and accidentally introduce a bug. Plus I think it might be possible to trigger the same problems as saving in insert mode with command sequences in normal mode
yeah I don't care for the saving in insertmode usecase that much but I do care about the sequence of commands (in normal mode) usecase
@the-mikedavis Feedback addressed, ready for re-review. I'll keep test driving but works well locally.
I've been daily driving this for about a week now and it fixes a bunch of stability issues i've been having :)
This improves the situation but I believe we need a larger change to make sure we're treating all execute_commands roughly the same. I have a patch that I'm working on locally in https://github.com/the-mikedavis/helix/commit/f7b2f770730cf9a8e0364affe9e78782b830080e that I will turn into a PR if I can iron out the bugs
@the-mikedavis any update on your end? I'll be rebasing my fork soon and would like to try out your patch, if it's finished. (though imo i think this should just be merged as-is and improved later...)
I pushed up the change here: https://github.com/helix-editor/helix/tree/append-changes-ensure-cursor-automatically
I made a few more changes before I pushed it so I'll try it out for a while before I turn it into a PR