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

Auto Save All Buffers After A Delay #9732

Closed
wants to merge 10 commits into from

Conversation

miggs597
Copy link
Contributor

@miggs597 miggs597 commented Feb 26, 2024

The feature I would like to introduce with this pull request is the ability to auto save one's work after idling. I've found it to be a great editing experience, especially while working with a stack that offers hot module reloading.

My implementation works similarly to idle_timer, that is once my save_delay_timer has reached zero an EditorEvent::SaveDelayTimer is returned causing EditorView::handle_event to handle that timeout and save the buffers.

Any feedback is greatly appreciated.

@woojiq
Copy link
Contributor

woojiq commented Feb 26, 2024

Sorry, I don't understand the use case if helix already has auto-save:

Enable automatic saving on the focus moving away from Helix.

Can you explain more about hot reloading usage? Do you want to automatically save every second or what?

@pascalkuthe
Copy link
Member

This is definitely useful but it shouldn't be implemented with the idle timeout we will remove that soonish (once all items in the tracking issue #9629 are done)

@kirawi
Copy link
Member

kirawi commented Feb 26, 2024

Resolves #3451

@miggs597
Copy link
Contributor Author

miggs597 commented Feb 26, 2024

Sorry, I don't understand the use case if helix already has auto-save:

Enable automatic saving on the focus moving away from Helix.

Can you explain more about hot reloading usage? Do you want to automatically save every second or what?

@woojiq Yes, helix does already have auto-save. However if enabled it's only triggered by changing focus away from helix. This change would allow for buffers to get saved after a certain amount of time has elapsed without any new input.

Before I was using vscode as my text editor and got used to it there. https://code.visualstudio.com/docs/editor/codebasics#_save-auto-save

For the hot module reloading stuff I was thinking about sveltekit, and how it's nice making changes to my frontend and seeing it near real time without having to navigate away from my editor. https://kit.svelte.dev/docs/faq#how-do-i-use-hmr-with-sveltekit

@miggs597
Copy link
Contributor Author

miggs597 commented Feb 26, 2024

This is definitely useful but it shouldn't be implemented with the idle timeout we will remove that soonish (once all items in the tracking issue #9629 are done)

@pascalkuthe Can you point me in the direction of some code samples that use the event system? I can work on updating my pr to use it instead of the current ad-hoc method.

@kirawi
Copy link
Member

kirawi commented Feb 26, 2024

https://github.com/helix-editor/helix/tree/master/helix-term/src/handlers contain them

@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 27, 2024
@miggs597 miggs597 marked this pull request as draft February 27, 2024 00:52
@miggs597
Copy link
Contributor Author

Moving to a draft for now. I've started working on porting the idea over to the event system.

@miggs597
Copy link
Contributor Author

miggs597 commented Feb 27, 2024

This is definitely useful but it shouldn't be implemented with the idle timeout we will remove that soonish (once all items in the tracking issue #9629 are done)

@pascalkuthe I realized that my initial pr description wasn't clear. I am not piggybacking off of idle timeout, however I do depend on the event_loop_until_idle. That's where it is being checked if the save timeout has been reached.

Is Application::event_loop_until_idle also being ported over to the event system, or is that code going to stay as is?
I'm trying to figure out where it would be most appropriate to dispatch an auto save event after a duration has expired.

@pascalkuthe
Copy link
Member

All of that is going away. Eberything is getting its own async handler with its own timout.

Moving around should not prevent autosave. You should be registering documentdidchange handler and start a (fairly long like 5s) timeout there that will trigger autosave when finished. Like kirawi said look at how the signature help handler is implemented (just that you don't care about mode and selection changes)

@miggs597
Copy link
Contributor Author

Initial port to the event system, implementation is currently half-baked. I need to iron out the user experience, and to get it working consistently.

Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

This is actually already a really good implementation. I have some minor comments but apart from that this seems reads to merge actually


#[derive(Debug)]
pub(super) struct AutoSaveHandler {
trigger: Option<AutoSaveInvoked>,
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to keep the trigger around , you don't actually use this

context.editor.set_error(format!("{}", e));
}

if let Err(e) = context.block_try_flush_writes() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think thsi should be here, this is only intended for shutdown. We don't want to freeze the editor while writing to disk

idle_timeout: Duration::from_millis(250),
save_delay_timeout: Duration::from_millis(1000),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the default should be longer like 5s?

Choose a reason for hiding this comment

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

For me the primary benefit of auto saving after a delay is updating lints from LSPs such as rust-analyzer. IMO one second is the best default for this use case. This also seems to be main use case mentioned on #3451. 1s is also the default in vscode and it works well there.

@pascalkuthe
Copy link
Member

closing in favor of #10899

@pascalkuthe pascalkuthe closed this Jun 8, 2024
@miggs597 miggs597 deleted the auto_save_on_delay branch July 14, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants