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

Delay auto-save until exiting insert mode #11047

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Conversation

the-mikedavis
Copy link
Member

@the-mikedavis the-mikedavis commented Jun 27, 2024

Saving while in insert mode causes issues with the modification indicator and this is very easy to reproduce with the current state of the auto-save hook. We can tweak the hook slightly to await the mode switch out of insert mode to perform the save.

The debounce is preserved: if you save and then immediately exit insert mode the debounce will be respected. If the debounce lapses while you are in insert mode, the save occurs as you switch out of insert mode immediately.

Fixes #10991

@the-mikedavis the-mikedavis added this to the 24.06 milestone Jun 27, 2024
Some(Instant::now() + Duration::from_millis(timeout))
match event {
Self::Event::DocumentChanged { debounce } => {
self.save_pending = true;
Copy link
Member

Choose a reason for hiding this comment

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

as far as I can see we never reset save_pending back to false. I guess that would need to happen when leaving insertmodel? This also sets this to true in normal mode would also cause excess/incorrect saves.

I actually had something else in mind:

  • make save_pending an Arc<AtomicBool>, only inside finish_deboucne/dispatch_blocking if the mode is insert mode store true in there (needs to be an Arc/AtomicBool so we can move it inside the callback)
  • Another clone of the arc is moved into the mode switch callback. When going from insert to normal mode that callback will check the value of the shared bool. If it's true it will instantly save (not send an even, save right in the mode switch handler) and then store false in the callback

I think doing it as an event that triggers a new save could work too. But I think the Arc/AtomicBool that only gets set to true if the calblack actually failed (so only we we are actually in normal mode when trying to save) is definitly necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yep I forgot to reset it. But I like the sound of the atomic bool more!

Copy link
Member Author

@the-mikedavis the-mikedavis Jun 27, 2024

Choose a reason for hiding this comment

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

I switched to the atomic bool so we can set it in the dispatch callback but I didn't also do the part about passing the bool to the mode switch hook. I'd like to keep the behavior where we delay even when switching modes rather than saving immediately on mode switch.

Maybe saving on mode switch should be a separate auto-save option though?

Copy link
Member

Choose a reason for hiding this comment

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

hmm so I wasn't necessarily thinking of always saving immediately on mode switch but rather:

  • the save should happen in insert-mode, the timeout already expired we just can't save right now due to internal limitations so we set a flag that an autosave is "pending"
  • as soon as we exiting insertmode we execute this "pending" write right away. We execute it right aways since it really should have executed earlier in insertmode, we just couldn't execute it due to internal limitations so we execute it first thing we get

(the pending flag would be the Arc<AtomicBool>)

this is kind of orthogonal to saving on mode switch/exiting insertmode in general. The main difference is that it's a bit more robust by not passing trough the timeout mechanism.

On the other hand I am not sure if savin in the mode switch hook is even possible. I think we only create the history entry after the mode switch hook executes so going trough the event handler is probably necessary/the right thing to do.

Copy link
Member Author

@the-mikedavis the-mikedavis Jun 27, 2024

Choose a reason for hiding this comment

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

Oh I see now, I misunderstood. I was also curious about the ordering and I believe you're correct that we need to go through the event handler so that we save to history before attempting to write.

edit: yep just tried it out and it still has the modification indicator problem sadly - that would be a more elegant solution

pascalkuthe
pascalkuthe previously approved these changes Jun 27, 2024
helix-term/src/handlers/auto_save.rs Outdated Show resolved Hide resolved
Saving while in insert mode causes issues with the modification
indicator and this is very easy to reproduce with the current state of
the auto-save hook. We can tweak the hook slightly to await the mode
switch out of insert mode to perform the save.

The debounce is preserved: if you save and then immediately exit insert
mode the debounce will be respected. If the debounce lapses while you
are in insert mode, the save occurs as you switch out of insert mode
immediately.
@archseer archseer merged commit dca952c into master Jun 29, 2024
6 checks passed
@archseer archseer deleted the auto-save-no-insert-mode branch June 29, 2024 02:08
the-mikedavis added a commit that referenced this pull request Jun 30, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Saving while in insert mode causes issues with the modification
indicator and this is very easy to reproduce with the current state of
the auto-save hook. We can tweak the hook slightly to await the mode
switch out of insert mode to perform the save.

The debounce is preserved: if you save and then immediately exit insert
mode the debounce will be respected. If the debounce lapses while you
are in insert mode, the save occurs as you switch out of insert mode
immediately.
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Aug 14, 2024
Saving while in insert mode causes issues with the modification
indicator and this is very easy to reproduce with the current state of
the auto-save hook. We can tweak the hook slightly to await the mode
switch out of insert mode to perform the save.

The debounce is preserved: if you save and then immediately exit insert
mode the debounce will be respected. If the debounce lapses while you
are in insert mode, the save occurs as you switch out of insert mode
immediately.
kyruzic pushed a commit to kyruzic/helix that referenced this pull request Sep 27, 2024
Saving while in insert mode causes issues with the modification
indicator and this is very easy to reproduce with the current state of
the auto-save hook. We can tweak the hook slightly to await the mode
switch out of insert mode to perform the save.

The debounce is preserved: if you save and then immediately exit insert
mode the debounce will be respected. If the debounce lapses while you
are in insert mode, the save occurs as you switch out of insert mode
immediately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

:q (quit) command unexpected behaviour (unsaved buffer) with auto-save by delay
3 participants