helix icon indicating copy to clipboard operation
helix copied to clipboard

Fix for saving from insert mode

Open omentic opened this issue 1 year ago • 11 comments

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.

omentic avatar Jun 04 '23 07:06 omentic

https://github.com/helix-editor/helix/pull/2883#pullrequestreview-1029452126

archseer avatar Jun 04 '23 14:06 archseer

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.

omentic avatar Jun 04 '23 16:06 omentic

Looking at the changes you only add doc.append_changes_to_history(view) into write_impl but not into write_all_impl.

Xalfer avatar Jun 05 '23 14:06 Xalfer

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.

David-Else avatar Feb 23 '24 15:02 David-Else

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

the-mikedavis avatar Feb 24 '24 15:02 the-mikedavis

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

pascalkuthe avatar Feb 24 '24 16:02 pascalkuthe

@the-mikedavis Feedback addressed, ready for re-review. I'll keep test driving but works well locally.

omentic avatar Feb 25 '24 04:02 omentic

I've been daily driving this for about a week now and it fixes a bunch of stability issues i've been having :)

Solsticely avatar Mar 28 '24 06:03 Solsticely

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 avatar Mar 28 '24 23:03 the-mikedavis

@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...)

omentic avatar Apr 22 '24 23:04 omentic

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

the-mikedavis avatar Apr 24 '24 12:04 the-mikedavis