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 #10899

Merged
merged 13 commits into from
Jun 10, 2024

Conversation

hw0lff
Copy link
Contributor

@hw0lff hw0lff commented Jun 7, 2024

This is a continuation of #9732

Auto save can be configured to trigger on focus loss:

auto-save.focus-lost = true # default: false

and after a time delay (in milli seconds) since last keypress:

auto-save.after-delay.enable = true # default: false
auto-save.after-delay.timeout = 300 # range: [0; u64::MAX] default: 3000

while preserving the old behavior of setting auto-save on focus loss with:

auto-save = true # default: false

I'm unsure if the current configuration design aligns with existing patterns.
Any feedback is greatly appreciated.

miggs597 and others added 8 commits June 7, 2024 13:12
- Remove unneccessary field
- Remove blocking save
Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```
@hw0lff hw0lff marked this pull request as draft June 7, 2024 11:38
@hw0lff hw0lff changed the title Draft: Auto Save All Buffers After A Delay Auto Save All Buffers After A Delay Jun 7, 2024
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.

functionality wise this lgtm but I had some comments about reducing unneded code a bit

helix-view/src/editor.rs Outdated Show resolved Hide resolved
helix-view/src/handlers/lsp.rs Outdated Show resolved Hide resolved
helix-term/src/handlers/auto_save.rs Outdated Show resolved Hide resolved
helix-view/src/editor.rs Outdated Show resolved Hide resolved
@hw0lff
Copy link
Contributor Author

hw0lff commented Jun 8, 2024

What meaning do we want to assign to setting only auto-save to a bool?
I see the following options:

true => enable auto save on focus-lost only
false => disable auto save on focus-lost only

or:

true => enable auto save on focus lost only
false => disable auto save entirely

or:

true => enable auto save on focus-lost and after-delay. also use the default timeout of after-delay.
false => disable auto save entirely

This decision depends on if we want to have auto save after delay enabled by default.
Does it make sense to enable auto-save after-delay by default if focus-lost is disabled by default?

IMO the most sensible and expected behavior of auto-save (as a bool) would be to act as a "global" switch, disabling or enabling all possible auto-save triggers together.

@pascalkuthe
Copy link
Member

By default auto saving after a delay should not be enabled that is too unexpected and can lead to lost work. The option to set auto-save to a bool is only needed/intended for backwards compatibility. Once we switch the config to scheme this legacy compatibility option will be removed so the point of keeping it is just to not break/change existing config.

If a user has auto-save = false or auto-save = true in their config it should work the same as it does today (no auto-save after delay and the bool controls whether to auto-save on focus lost). So auto-save = true should only enable the focus lost option.

Any user that wants to use the new auto-save option should explicitly opt into it by using the new syntax. I would also remove the old syntax from the docs and only document the new options (in general you need to update the docs in book/editor.md).

@hw0lff
Copy link
Contributor Author

hw0lff commented Jun 8, 2024

If a user has auto-save = false or auto-save = true in their config it should work the same as it does today (no auto-save after delay and the bool controls whether to auto-save on focus lost).

Any user that wants to use the new auto-save option should explicitly opt into it by using the new syntax.

Yeah, this makes more sense.

@hw0lff
Copy link
Contributor Author

hw0lff commented Jun 8, 2024

Ready for review. Anything else?

@pascalkuthe pascalkuthe marked this pull request as ready for review June 8, 2024 20:40
@pascalkuthe
Copy link
Member

You still need to update the documentation in the book (book/src/editor.md) but apart from that this looks good I think

@hw0lff
Copy link
Contributor Author

hw0lff commented Jun 8, 2024

I pushed the updated docs.

book/src/editor.md Outdated Show resolved Hide resolved
pascalkuthe
pascalkuthe previously approved these changes Jun 8, 2024
@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 8, 2024
@kirawi
Copy link
Member

kirawi commented Jun 9, 2024

Will resolve #3451

book/src/editor.md Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis linked an issue Jun 10, 2024 that may be closed by this pull request
@pascalkuthe pascalkuthe merged commit 265608a into helix-editor:master Jun 10, 2024
6 checks passed
mxxntype pushed a commit to mxxntype/helix that referenced this pull request Jun 12, 2024
* auto save after delay

* configable

* clearer names

* init

* working with some odd behaviour

* working with greater consistency

* Apply reviewer suggestions

- Remove unneccessary field
- Remove blocking save

* Improve auto-save configuration

Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```

* Remove boilerplate and unnecessary types

* Remove more useless types

* Update docs for auto-save.after-delay

* Fix wording of (doc) comments relating to auto-save

* book: Move auto-save descriptions to separate section

---------

Co-authored-by: Miguel Perez <[email protected]>
Co-authored-by: Miguel Perez <[email protected]>
AOx0 pushed a commit to AOx0/helix that referenced this pull request Jun 15, 2024
* auto save after delay

* configable

* clearer names

* init

* working with some odd behaviour

* working with greater consistency

* Apply reviewer suggestions

- Remove unneccessary field
- Remove blocking save

* Improve auto-save configuration

Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```

* Remove boilerplate and unnecessary types

* Remove more useless types

* Update docs for auto-save.after-delay

* Fix wording of (doc) comments relating to auto-save

* book: Move auto-save descriptions to separate section

---------

Co-authored-by: Miguel Perez <[email protected]>
Co-authored-by: Miguel Perez <[email protected]>
AOx0 pushed a commit to AOx0/helix that referenced this pull request Jun 23, 2024
* auto save after delay

* configable

* clearer names

* init

* working with some odd behaviour

* working with greater consistency

* Apply reviewer suggestions

- Remove unneccessary field
- Remove blocking save

* Improve auto-save configuration

Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```

* Remove boilerplate and unnecessary types

* Remove more useless types

* Update docs for auto-save.after-delay

* Fix wording of (doc) comments relating to auto-save

* book: Move auto-save descriptions to separate section

---------

Co-authored-by: Miguel Perez <[email protected]>
Co-authored-by: Miguel Perez <[email protected]>
AOx0 pushed a commit to AOx0/helix that referenced this pull request Jun 27, 2024
* auto save after delay

* configable

* clearer names

* init

* working with some odd behaviour

* working with greater consistency

* Apply reviewer suggestions

- Remove unneccessary field
- Remove blocking save

* Improve auto-save configuration

Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```

* Remove boilerplate and unnecessary types

* Remove more useless types

* Update docs for auto-save.after-delay

* Fix wording of (doc) comments relating to auto-save

* book: Move auto-save descriptions to separate section

---------

Co-authored-by: Miguel Perez <[email protected]>
Co-authored-by: Miguel Perez <[email protected]>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* auto save after delay

* configable

* clearer names

* init

* working with some odd behaviour

* working with greater consistency

* Apply reviewer suggestions

- Remove unneccessary field
- Remove blocking save

* Improve auto-save configuration

Auto save can be configured to trigger on focus loss:
```toml
auto-save.focus-lost = true|false
```

and after a time delay (in milli seconds) since last keypress:
```toml
auto-save.after-delay.enable = true|false
auto-save.after-delay.timeout = [0, u64::MAX] # default: 3000
```

* Remove boilerplate and unnecessary types

* Remove more useless types

* Update docs for auto-save.after-delay

* Fix wording of (doc) comments relating to auto-save

* book: Move auto-save descriptions to separate section

---------

Co-authored-by: Miguel Perez <[email protected]>
Co-authored-by: Miguel Perez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-save
5 participants