Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for saving from insert mode #7226

Closed
wants to merge 1 commit into from

Conversation

omentic
Copy link
Contributor

@omentic omentic commented Jun 4, 2023

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 e2a8774 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.

@archseer
Copy link
Member

archseer commented Jun 4, 2023

#2883 (review)

@omentic
Copy link
Contributor Author

omentic commented Jun 4, 2023

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.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. C-bug Category: This is a bug labels Jun 5, 2023
@Xalfer
Copy link
Contributor

Xalfer commented Jun 5, 2023

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

helix-term/src/ui/editor.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 18, 2024
@David-Else
Copy link
Contributor

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. #2883 (review) 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.

@the-mikedavis
Copy link
Member

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

@pascalkuthe
Copy link
Member

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

@omentic
Copy link
Contributor Author

omentic commented Feb 25, 2024

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

@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2024
@Solsticely
Copy link

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

@the-mikedavis
Copy link
Member

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 the-mikedavis@f7b2f77 that I will turn into a PR if I can iron out the bugs

@omentic
Copy link
Contributor Author

omentic commented Apr 22, 2024

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

@the-mikedavis
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
8 participants