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.

Sign up for GitHub

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

Jump to bottom

save backup at user selected path #99

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

save backup at user selected path #99

wants to merge 6 commits into from

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/DataComponents.kt Outdated Show resolved Hide resolved
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

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
if (backupJob == null) {
backupJob = viewModelScope.launch(ioDispatcher) {
_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

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

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

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

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