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 :w in insert mode #2883

Closed
wants to merge 0 commits into from
Closed

Conversation

xDarksome
Copy link

@xDarksome xDarksome commented Jun 26, 2022

Currently, when you bind "C-s" = ":w" in insert mode and try to use it, the file is being written, but changes are considered not committed yet. Because of that helix complains on :q and still shows [+] in the status bar.

Tell me if there is better way to fix this.

@feep
Copy link

feep commented Jun 28, 2022

Nice fix, thanks.

I was unable to bind anything to ":w" in insert, was unsure if I was doing it wrong or if it was not currently possible with keybinds in config.toml.

The following config.toml binding does not work in 22.05, does work with this PR:

[keys.insert]
C-s = ":w"

@the-mikedavis
Copy link
Member

I can't reproduce that - running exactly that configuration file on 22.05 works and C-s successfully saves a file in insert mode (although it does not clear the modified indicator). You may have other bindings or configuration options that are invalid in your config.toml.

@feep
Copy link

feep commented Jun 29, 2022

running exactly that configuration file on 22.05 works and C-s successfully saves a file in insert mode (although it does not clear the modified indicator).

That is correct.

I think we are all describing the same behavior. Maybe I wasn't clear.

As described by you and xDarksome. v22.05 does not clear the dirty bit (It does save the file).

Didn't actually check to see if it saved the file, because that wasn't the bug.

...checking now.

With my config and v22.05, file is saved, but file is still shown as dirty.

With minimal config [keys.insert]\nC-s = ":w", same result.

I'm pretty sure that is the same behavior the-mikedavis is describing.

If we still conflict, more info about my build:

❯ rustc --version
rustc 1.61.0 (Arch Linux rust 1:1.61.0-1)

❯ ./target/release/hx --version # PR version, resets dirty flag properly
helix 22.05 (a7f41ac5)

❯ uname -a
Linux [hostname] 5.15.48-1-lts #1 SMP Thu, 16 Jun 2022 13:32:35 +0000 x86_64 GNU/Linux

@the-mikedavis
Copy link
Member

Oh I see, I mis-parsed your comment as saying that helix was rejecting that keybinding config 😅

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will quite likely be fixed by #2267

\cc @dead10ck ?

Comment on lines 849 to 851
if let Some(view_id) = view_id {
transaction = transaction.with_selection(self.selection(view_id).clone());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't correct, the code assumes there's always a selection so we have the previous selection stored when undoing. The editor will probably panic otherwise.

Copy link
Member

@dead10ck dead10ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @archseer. I assume this was done because in the save function call, you don't have a view associated with it. The solution isn't to just do whatever you can without it and hope it works out. This will leave the last edit in the history without a selection, which at best will make a very confusing and invisible undo operation, and at worst make the editor crash.

#2267 will not fix this either. The reason this is happening is because the modified flag is controlled by the revision number, which gets changed every time we commit changes to the history of edits for the purposes of tracking undos, and then make further changes. When the document's revision number is equal to the last revision number we saved to disk, the state is unmodified.

The way helix works as a modal editor is that you make your edits by going into insert mode, making your edits, and then leaving insert mode to go back to normal mode. This cycle of normal mode → insert mode → normal mode is the unit of discrete state changes to the text, and it's during the state transition from insert mode to normal mode that changes are recorded.

By trying to commit the changes while still in insert mode, you are breaking some key assumptions about how state changes are tracked. You shouldn't do it.

Your life would be made easier if you actually adopted the modal editing paradigm, instead of trying to make a modal editor into a non-modal editor.

@danielrode
Copy link

Workaround:

[keys.insert]
C-s = ["commit_undo_checkpoint", ":write"]

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 S-needs-discussion Status: Needs discussion or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants