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

PICARD-2875: Let the user select the auto backup directory in Maintenance Options #2430

Merged
merged 11 commits into from
Apr 25, 2024

Conversation

zas
Copy link
Collaborator

@zas zas commented Apr 23, 2024

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

That's the directory used to saved configuration backups (auto & user).
Currently it is defaulting to Documents (depending on platform), but the user cannot change it.

Solution

Provide a way to select target directory and remember it between runs.
It is stored in a new setting named autobackup_directory.

image

Action

Additional actions required:

  • Update Picard documentation (please include a reference to this PR)
  • Other (please specify below)

@zas zas requested a review from rdswift April 23, 2024 12:45
@zas
Copy link
Collaborator Author

zas commented Apr 23, 2024

@rdswift please test this and feel free to improve, the idea is to let the user select the target directory instead of always loading the default one.

@zas zas force-pushed the save_last_backup_dir branch from 45bb68f to fc5f89b Compare April 23, 2024 16:29
@zas zas requested a review from phw April 23, 2024 16:39
@zas zas force-pushed the save_last_backup_dir branch from 7111529 to 2aa5561 Compare April 24, 2024 13:49
@zas zas marked this pull request as ready for review April 24, 2024 14:10
Copy link
Collaborator

@rdswift rdswift left a comment

Choose a reason for hiding this comment

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

One other thing I noticed while testing was that if a non-existent directory was specified in the backup directory text box (for example /home/user/Documents/PicardBackups) and the settings saved with Make It So!, the setting (text box) would silently reset to the default. Perhaps when exiting the text box we could check if the specified directory exists and if not, pop up a dialog asking if the user wanted to create it. If the user selects to create it and it can't be created, then a dialog box should indicate that there was a problem. In that case, or if the user chooses to cancel creating the directory, then it should revert to the previously saved value (or the default value). In any case, it probably shouldn't reset to the default value silently.

picard/ui/options/maintenance.py Outdated Show resolved Hide resolved
picard/ui/ui_options_maintenance.py Outdated Show resolved Hide resolved
@zas
Copy link
Collaborator Author

zas commented Apr 24, 2024

``, the setting (text box) would silently reset to the default.

Yes, I'm aware of it, I didn't want to make the thing too complicated. Instead of reverting to default we could just ignore the change.

@rdswift
Copy link
Collaborator

rdswift commented Apr 24, 2024

Instead of reverting to default we could just ignore the change.

With notification? My concern is that something that the user entered (even if wrong) will be silently modified, so what is saved is not what they input and they aren't made aware of the change.

@zas
Copy link
Collaborator Author

zas commented Apr 24, 2024

With notification? My concern is that something that the user entered (even if wrong) will be silently modified, so what is saved is not what they input and they aren't made aware of the change.

I added a notification and revert back to last valid path if it happens, not perfect, but better than nothing. A new directory can be created from the directory selector, so I don't think we should make things too complex and let the user creates the dir from this warning dialog.
See f26a74f

- expand automatic backup directory label and add trailing colon
- move Load and Save buttons
- add line to separate config file/directory from setting removal
- change to sentence case for existing option label and buttons
- add ellipses to buttons that open new dialogs/windows
rdswift
rdswift previously approved these changes Apr 24, 2024
Copy link
Collaborator

@rdswift rdswift left a comment

Choose a reason for hiding this comment

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

I submitted a PR for some suggested UI changes for your consideration. Even if you decide not to include them, or they should be in a separate PR, I'm still good with this because I believe the functionality is appropriate as you have it implemented. Thanks.

Modify maintenance options page label text and layout
@zas
Copy link
Collaborator Author

zas commented Apr 24, 2024

I submitted a PR

It looks good to me, merged, thanks!

@rdswift
Copy link
Collaborator

rdswift commented Apr 24, 2024

On that page I would also like to change the logic (and possibly layout slightly) so that you don't have to select the checkbox to view (scroll) the list of unused options. It seems I really didn't think the UI through as well as I should have when I set it up initially. 😊 In any event, I'll save that for a new PR later because it doesn't really apply to the subject of this PR.

@zas zas changed the title Let the user select the auto backup directory in Maintenance Options PICARD-2875: Let the user select the auto backup directory in Maintenance Options Apr 24, 2024
@zas zas merged commit 4bfbe01 into metabrainz:master Apr 25, 2024
41 checks passed
@zas zas deleted the save_last_backup_dir branch April 25, 2024 09:10
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.

2 participants