Skip to content

save backup at user selected path#99

Open
am-ghaseminia wants to merge 6 commits intodevfrom
dev_save_backup_at_user_selected_path
Open

save backup at user selected path#99
am-ghaseminia wants to merge 6 commits intodevfrom
dev_save_backup_at_user_selected_path

Conversation

@am-ghaseminia
Copy link
Collaborator

@am-ghaseminia am-ghaseminia commented Sep 29, 2023 •
edited
Loading

Description and Impact

Please include a summary of the changes and the related issue. Please also have relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • This change requires a documentation update
  • Refactor

Result

mehdiyari
mehdiyari reviewed Sep 30, 2023
View reviewed changes
features/backup/src/main/java/ir/mehdiyari/krypt/backup/DataRoute.kt Outdated
val context = LocalContext.current

var selectedDirectory: DocumentFile? by remember { mutableStateOf(null) }
val launcher = rememberLauncherForActivityResult(
Copy link
Owner

@mehdiyari mehdiyari Sep 30, 2023

Choose a reason for hiding this comment

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

Would you please rename this variable to something more meaningful based on the job of the variable?

features/backup/src/main/java/ir/mehdiyari/krypt/backup/DataViewModel.kt Outdated
_backupViewState.emit(BackupViewState.Started)
try {
if (backupRepository.backupAll()) {
if (backupRepository.backupAll(uri)) {
Copy link
Owner

@mehdiyari mehdiyari Sep 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please use uri to path inside view model, if we do this, there is no need to depend the data layer to the android related classes.

@mehdiyari mehdiyari linked an issue Sep 30, 2023 that may be closed by this pull request
Get the backup path from the user #96
Open
@mehdiyari
Copy link
Owner

mehdiyari commented Oct 1, 2023

Hey @am-ghaseminia, Please also test the whole flow with different scenarios like changing the state of the app to foreground and background, screen orientation, etc.

@mehdiyari
Copy link
Owner

mehdiyari commented Oct 5, 2023

I tested the flow and I found this issue.

  1. We should show the message to the user that the backup was saved to this folder, but we don't.
  2. Once the user clicks on the save button, we duplicate the backup, the behavior here should be, to open the choosing directory again, and move the current backup to new path.

@am-ghaseminia
Copy link
Collaborator Author

am-ghaseminia commented Oct 28, 2023

I tested the flow and I found this issue.

  1. We should show the message to the user that the backup was saved to this folder, but we don't.
  2. Once the user clicks on the save button, we duplicate the backup, the behavior here should be, to open the choosing directory again, and move the current backup to new path.

@mehdiyari jan, you are right about the first one, but about the second one, maybe user wants to have backup in different folders. did you consider this too?

@mehdiyari
Copy link
Owner

mehdiyari commented Dec 30, 2023

Hey @am-ghaseminia, any update about this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mehdiyari mehdiyari mehdiyari left review comments

Assignees

No one assigned

Labels

Feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Get the backup path from the user

2 participants

@am-ghaseminia @mehdiyari