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

File Recovery #651

Open
wants to merge 66 commits into
base: main
Choose a base branch
from
Open

File Recovery #651

wants to merge 66 commits into from

Conversation

LeSnake04
Copy link
Contributor

@LeSnake04 LeSnake04 commented Apr 26, 2023

closes #545

This adds an automatic save for all files that get shown to the user when the program crashes.

DISCUSSIONS:

  • separate timers for recovery and autosave?
  • do we need a recovery save indicator

TODO:

  • fix crash at launch
  • preserve state of file recovery state/delay settings between restarts
  • recovery dialog
  • Recovery info dialog
  • open file in new tab
  • fix always saving recovery save
  • fix recovery unsaved
  • remove recovery saves when exiting gracefully

@@ -302,15 +302,25 @@ impl RnCanvas {

if let Err(e) = res {
self.set_save_in_progress(false);
if self.recovery_in_progress() {
self.set_recovery_in_progress(false)
}

// If the file operations failed in any way, we make sure to clear the expect_write flag
// because we can't know for sure if the output_file monitor will be able to.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flxzt How important if this, when the user isn't supposed to access the folder where the file gets saved (as in the case of recovery files). Do I need to make another monitor for the recovery file?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I didn't look at the code too closely so far since this you marked it as a draft, I don't really know

@flxzt
Copy link
Owner

flxzt commented Jun 5, 2023

I just saw that the recovery mechanism is essentially a second autosave feature, even with a second indicator visible to the user.

I am sceptical if it should be done this way. What in my opinion would be a better implementation; save a file when unrecoverable errors are detected or a crash/panic can somehow be caught (using catch unwind maybe?).
If this is not possible, a periodic save should at least be hidden from the user, anything else is confusing. There is also the concern about the save time because saving takes quite a while currently for very large documents.

And another important part here is to display a dialog when a crash happened on startup (maybe check if crash-save files exists in a dedicated directory, we should probably use directories::data_dir() to adhere to xdg specification here) and asks if those files should be recovered.

@LeSnake04
Copy link
Contributor Author

LeSnake04 commented Jun 5, 2023

I am currently moving the indicator to a less obvious place, so the users don't get confused, but still can confirm it if interested (to the recovery settings, to be specific)

I am not sure if we can realistic catch all errors and this won't cover full system crashes at all. So I don't think there is an alternative to a second autosave, unfortunately. We can consider don't do recovery saving when the file is saved and autosave is enabled.
I will definitely turn off recovery if both the recovery and autosave timers are the same, since this caused issues in testing already.

The Dialog is planned. I will do something similar to the edit workspace dialog. I will work on that after I moved the indicator. The metadata is stored to provide additional context to the user when restoring.

edit: I am currently using dirs::data_dir()/rnote

Update: I paused recovery when file saved and recovery enabled.

@LeSnake04
Copy link
Contributor Author

I tried to make the confirm overwrite document dialog as generic as possible so it can be reused for something like the filebrowser.

@flxzt
Copy link
Owner

flxzt commented Oct 1, 2023

Alright, I like the recovery dialog, it provides useful options for the to-be-recovered data. There are some improvements that can be made with regards to the user experience, but it already roughly does what I think users would expect.

I am still sceptical about exposing options to the user. When would a user want to disable the recovery mechanism? If we have this feature, it should be enabled all the time and work in the background.

And I think it still is worth exploring if it is possible to wrap the entire application in a catch_unwind. Here a small snippet to illustrate how I think it could work:

use std::sync::{Arc, RwLock};

fn main() -> std::thread::Result<()> {
    let state = Arc::new(RwLock::new(String::from("no need to panic.")));
    let res = std::panic::catch_unwind(|| {
        will_panic(Arc::clone(&state));
    });
    println!("this state can be saved into a file for recovery: {}", state.read().unwrap());
    res
}

fn will_panic(state: Arc<RwLock<String>>) {
    *state.write().unwrap() = String::from(
        "
    this can be periodically written to with recovery state.
    As long as the panic does not happen while we write to the RwLock,
    the state will be accessible after the panic is caught in catch_unwind",
    );
    panic!();
}

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.

auto save shall also save drafts
2 participants