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

Update file change history before calling MappableCommand #4780

Closed
wants to merge 3 commits into from

Conversation

swork1
Copy link

@swork1 swork1 commented Nov 16, 2022

Before we run a mappable command we should update the current changes to history.

This fixes #4719

@the-mikedavis
Copy link
Member

Alternatively, keybinds that call typable commands from insert-mode could include commit_undo_checkpoint before the call to save

@the-mikedavis
Copy link
Member

I meant that it could be done in keymaps:

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

but I suppose there isn't really a downside to doing this for all typable commands. Mostly this will just be a no-op since you need to enter normal/select modes to execute typable commands and that also saves a checkpoint

@archseer
Copy link
Member

This breaks how the undo is supposed to work though, we've had this issue & PR before: #2883 (review) #3501

@archseer
Copy link
Member

I think this needs some discussion but maybe having the undo state broken into two undo steps is preferable to having this broken.

@@ -167,6 +167,7 @@ impl MappableCommand {
Self::Typable { name, args, doc: _ } => {
let args: Vec<Cow<str>> = args.iter().map(Cow::from).collect();
if let Some(command) = typed::TYPABLE_COMMAND_MAP.get(name.as_str()) {
commit_undo_checkpoint(cx);
Copy link
Member

Choose a reason for hiding this comment

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

Should only be called if in insert mode, together with a comment explaining why this is needed in the first place

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. R-breaking-change This PR is a breaking change for some behavior C-bug Category: This is a bug labels Nov 19, 2022
@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. R-breaking-change This PR is a breaking change for some behavior labels Nov 19, 2022
@pascalkuthe pascalkuthe added the S-needs-discussion Status: Needs discussion or design. label Jan 18, 2023
@pascalkuthe
Copy link
Member

I don't think this is the right solution to the problem. Some commands are fine to call from insertmode with creating a checkpoint. These commands should not be changed to create a checkpoint.

closing this one out as stale, thank you for contributing!

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-needs-discussion Status: Needs discussion or design. S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file-name element doesn't update when saving under INS mode
5 participants