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

Commit to history after executing a command from the palette #5294

Merged

Conversation

nrabulinski
Copy link
Contributor

Fixes #5261

While this does fix the underlying issue of that bug, which was that appending to history only happened in editor.rs:1424 which meant that undo-ing after executing a command from the palette would call undo before the change was committed, it does not address the bigger issue - that it's way too easy to mutate a document and have document and history desync.
I'm opening this PR not necessairly to have this fix merged (although we could do it to fix the issue temporarily) but to document and highlight the root cause and possibly discuss a long-term fix, which I'd gladly take on implementing.

Discussion from the matrix channel about this issue:

Pascal Kuthe
Generally any transaction should be appended immidietly to the history. We had quite a few bugs caused from missing that. You would need to find out how a history entry is normally created for a command and why its not happening from the picker.

I do not know the commands area of the editor aswell as I would like (I usually focus more on the render system with what I work on). Therefore I can't tell you how it works off the top of my head without looking at the source (and I am at a Christmas party rn :D) but looking around a bit could be a good way to get into the codebase a bit.
The interesting part here is that the same commands append to history when called normally but not when called trough the picker, if I understood the issue correctly
So I taught it would be some generic mechanism

niko (me)
Yeah the problem here is that we append to history after handling event in the Component impl, but that doesn’t get executed for the picker callback so there’s a couple options for solving this issue:
• Adding the code for appending history changes to command picker
• Adding the code for appending history changes to MappableCommand::execute
• Adding the code for appending history changes somewhere else entirely?

Ideally we wouldn’t want to repeat the same code multiple times in multiple places which is why I feel reluctant to just pasting that code into command palette handler but maybe that is the way forward

gabydd
Some commands like format make changes to the document asynchronously so they have to manually append that to the history
If I remember correctly we don't want to append to history if we are still in insert mode only on change to normal mode but that might be able to be checked not sure, Micheal Davis or archseer would probably have know what the best way to do this is

archseer
yeah that's how it works, I also was thinking commands/keybinds could return an Option<Transaction> that would get auto applied, but that's slightly more code in commands that don't do anything
and you wouldn't be able to apply multiple in a row then

@the-mikedavis
Copy link
Member

I think the ideal fix for this has some overlap with #4013, #4244 and #4709: we should be able to queue up command(s) for the compositor to execute in the same way that we can queue up key events. I'm not sure how that should look yet though.

This should fix most of the behavior with the command palette though so we might want to merge this as-is.

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 28, 2022
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I left some notes on an idea for a long-term fix: #4709 (comment) (note: that would be a very large change that should probably be split across multiple PRs).

It's pretty easy to trigger panics without this fix though so I'll pull this in as-is 👍

@the-mikedavis the-mikedavis changed the title Added commiting to history after executing a command from the palette Commit to history after executing a command from the palette Jan 16, 2023
@the-mikedavis the-mikedavis merged commit 3cf5216 into helix-editor:master Jan 16, 2023
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-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kill_to_line_end + undo crashs helix
3 participants