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 saving #412

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

MartijnvAdrichem
Copy link

This is a continuation of the discussion in the draft PR here
This fixes issue 22

How it works:

  • There is a new setting to enable/disable autosaves (it is enabled by default)
    afbeelding
  • When the focus on the YAFC window is lost an autosave is made (only when there anything got changed)
  • It uses a rolling window of 5 saves to keep a small history.

When loading a project there is a new setting:
afbeelding
When disabled it will just load the save you selected (whether it is your main save or an autosave doesn't matter), when it is enabled it will try to find the most recent save you have.
afbeelding
In the case above autosave-1.yafc would be loaded eventhough the user selected "project-yafc".

When manually saving you will always save the 'main' file (project.yafc in the above case), this because autosaves get overwritten real fast, so when you would open an auto-save and forget to change the save path you would quickly lose that manual save.

There is one 'edge' case in which the user manually changes the filename of the autosave, but this wont break anything.

Let me know what you guys think, if everyone agrees that this is a solid solution then I will add some tests.

@MartijnvAdrichem MartijnvAdrichem changed the title Feature/autosave Auto saving Feb 11, 2025
Copy link
Collaborator

@veger veger left a comment

Choose a reason for hiding this comment

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

It looks nice and simple, I like it!

Some general feedback (I didn't fully check the code yet):

  • When people disable timestamps on their files to save some (SSD) wear and tear, detecting the newest autosave is not going to work. Why not keep on counting, instead of going back to 1? Then you just grab the highest number autosave. (yes, we'd have to actively delete the lowest number)
  • Is there another edge case when the project is saved? When you load it again, the autosaves should not be loaded as they are older...? (Maybe it works because the focus will be lost after saving and quitting, so a quick autosave is dropped? 🤔 )
  • There are quite a bunch of unrelated (mostly? formatting) changes in this PR.
  • Missing changelog entry (can be done just before merging though)

For me this whole idea is good enough, and I think for the others as well. So feel free to clean up and we'll fully review and accept when all is addressed.

@shpaass shpaass linked an issue Feb 11, 2025 that may be closed by this pull request
@MartijnvAdrichem
Copy link
Author

@veger I will check those points tomorrow :)

About the timestamp, that is unfortunate. Keeping an infinite counter would fix that, but would make it significant more complex (deleting older files, more difficult to find the highest save file, because AFAIK it is not easy to search the filesystem with regex.)

Maybe we would need to accept that this minorty should manually select te right one (disabling the checkbox when loading the save)

@MartijnvAdrichem
Copy link
Author

@veger

It looks nice and simple, I like it!

Some general feedback (I didn't fully check the code yet):

* When people disable timestamps on their files to save some (SSD) wear and tear, detecting the newest autosave is not going to work. Why not keep on counting, instead of going back to 1? Then you just grab the highest number autosave. (yes, we'd have to actively delete the lowest number)

* Is there another edge case when the project is saved? When you load it again, the autosaves should not be loaded as they are older...? (Maybe it works because the focus will be lost after saving and quitting, so a quick autosave is dropped? 🤔 )

* There are quite a bunch of unrelated (mostly? formatting) changes in this PR.

* Missing changelog entry (can be done just before merging though)

For me this whole idea is good enough, and I think for the others as well. So feel free to clean up and we'll fully review and accept when all is addressed.

  • timestamps see reply above

  • I do not completely follow you on that edge case. If your main save file is the newest then that one will get chosen instead:
    afbeelding

  • I tried to fix the formatting changes.

  • I added the changelog entry.

@veger
Copy link
Collaborator

veger commented Feb 13, 2025

I do not completely follow you on that edge case. If your main save file is the newest then that one will get chosen

Ah... you also read the timestamp of the original file? Then it should be fine! (I didn't fully check your code yet)

When I have some (more) time I'll do a proper review.

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.

Autosave by default
2 participants