Single backup #1439
Single backup #1439
Conversation
There was a problem hiding this comment.
Thanks for picking this up!
I only took a cursory look at the code, because I think we first need to make some adjustments to the flow here. Instead of introducing a new "Single backup" toggle, repurpose the "Number of versions to keep" option as a "Versioning strategy" setting as proposed here: #745 (comment). "None" implies a single file and "Keep x versions" implies a directory. Prompt the user to make a choice when first enabling automatic backups. If the user changes the versioning strategy later from single file to directory or the other way around, force the user to select a new location. "Directory for backup files" becomes "Backup location", etc.
We also need some kind of warning (with the usual "I understand the risk" checkbox) when a user tries to opt for a single file backup. Something like: "The selected versioning strategy is not reliable and not recommended. A single backup failure could lead to losing your only backup."
| @@ -562,6 +567,25 @@ public Set<UUID> getGroupFilter() { | |||
| } | |||
| } | |||
|
|
|||
| @Nullable | |||
| public Uri getBackupFileLocation() { | |||
| String value = _prefs.getString("pref_backup_file_location", null); | |||
There was a problem hiding this comment.
I don't think we need a separate preference for this. We should be able to detect whether we're dealing with a directory or a single file based on the URI:
- content://com.android.externalstorage.documents/tree
- content://com.android.externalstorage.documents/document
r3dh3ck
force-pushed
the
feature/single_backup
branch
from
1a9ce5c
to
4225e89
Compare
|
|
|
Thanks for the update! Looks good. I'll take a closer look soon. Hopefully later this week. |
| <string name="pref_backups_versioning_strategy_keep_x_versions">Keep x versions</string> | ||
| <string name="pref_backups_versioning_strategy_single_backup">Single backup</string> | ||
| <string name="pref_backups_versioning_strategy_single_backup_warning">The selected versioning strategy is not reliable and not recommended. A single backup failure could lead to losing your only backup.</string> | ||
| <string name="pref_backups_versioning_strategy_dialog_title">Select your desired versioning strategy</string> |
There was a problem hiding this comment.
I'd suggest: "Select a versioning strategy"
| @@ -221,6 +244,15 @@ private CharSequence getBackupStatusMessage(@Nullable Preferences.BackupResult r | |||
| return spannable; | |||
| } | |||
|
|
|||
| private void createBackupFile() { | |||
| String fileName = VaultBackupManager.FILENAME_PREFIX +".json"; | |||
There was a problem hiding this comment.
Please add the following below the definition of VaultBackupManager.FILENAME_PREFIX and refer to it here:
public static final String FILENAME_SINGLE = String.format("%s.json", FILENAME_PREFIX);| } | ||
| ContentResolver resolver = _context.getContentResolver(); | ||
| try (FileInputStream inStream = new FileInputStream(tempFile)) { | ||
| OutputStream outStream = resolver.openOutputStream(fileUri, "wt"); |
There was a problem hiding this comment.
Include the opened OutputStream in the try-with-resources statement above.
| @@ -51,10 +53,10 @@ public VaultBackupManager(Context context, AuditLogRepository auditLogRepository | |||
| _auditLogRepository = auditLogRepository; | |||
| } | |||
|
|
|||
| public void scheduleBackup(File tempFile, Uri dirUri, int versionsToKeep) { | |||
There was a problem hiding this comment.
We should keep passing these settings in instead of reading from Preferences here. There's a potential race where the preferences can change while the backup is running.
| } else if (strategy == BackupsVersioningStrategy.MULTIPLE_BACKUPS) { | ||
| int versionsToKeep = _prefs.getBackupsVersionCount(); | ||
| createBackup(tempFile, uri, versionsToKeep); | ||
| } |
There was a problem hiding this comment.
Throw a RuntimeException if the strategy is anything other than SINGLE_BACKUP or MULTIPLE_BACKUPS.
| _versioningStrategyPreference.setOnPreferenceClickListener(preference -> { | ||
| BackupsVersioningStrategy currentStrategy = _prefs.getBackupVersioningStrategy(); | ||
| Dialogs.showBackupsVersioningStrategy(requireContext(), currentStrategy, strategy -> { | ||
| if (strategy == BackupsVersioningStrategy.MULTIPLE_BACKUPS) { |
There was a problem hiding this comment.
Let's make sure the user only has to select a location if the versioning strategy was actually changed.
| String path = uri.getPath(); | ||
| if (path == null) { | ||
| return BackupsVersioningStrategy.UNDEFINED; | ||
| } | ||
| String[] pathArray = path.split("/"); | ||
| if (pathArray.length < 2) { | ||
| return BackupsVersioningStrategy.UNDEFINED; | ||
| } | ||
| String type = pathArray[1]; | ||
| if (type.equals("tree")) { | ||
| return BackupsVersioningStrategy.MULTIPLE_BACKUPS; | ||
| } else if (type.equals("document")) { | ||
| return BackupsVersioningStrategy.SINGLE_BACKUP; | ||
| } |
There was a problem hiding this comment.
I found a nicer way to do this check: DocumentsContractCompat.isTreeUri
Resolves #745
