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

[FEATURE REQUEST] Copy/move conflict solved by users #4062

Merged
merged 15 commits into from
Jun 21, 2023
Merged

[FEATURE REQUEST] Copy/move conflict solved by users #4062

merged 15 commits into from
Jun 21, 2023

Conversation

manuelplazaspalacio
Copy link
Contributor

@manuelplazaspalacio manuelplazaspalacio commented Jun 7, 2023 •
edited
Loading

Related Issues

App: #3959

Library PR (if needed): owncloud/android-library#571

  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

Test Plan: https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_4.1/Conflicts%20while%20copy.md

Reports and improvements:

@manuelplazaspalacio manuelplazaspalacio changed the title Feature/copy move conflic solved users [FEATURE REQUEST] Copy/move conflict solved by users Jun 7, 2023
manuelplazaspalacio added a commit that referenced this pull request Jun 7, 2023
@manuelplazaspalacio
Change name changelog to #4062.
f78db66
@manuelplazaspalacio manuelplazaspalacio self-assigned this Jun 7, 2023
@manuelplazaspalacio manuelplazaspalacio linked an issue Jun 7, 2023 that may be closed by this pull request
[FEATURE REQUEST] Copy/move conflict solved by users #3935
Closed
13 tasks
JuancaG05
JuancaG05 requested changes Jun 8, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Good job @manuelplazaspalacio! Some comments here 🚀
Also, don't forget to add your name in the @author tag of every class you modify in their header 😉

changelog/unreleased/4062 Outdated Show resolved Hide resolved
changelog/unreleased/4062 Outdated Show resolved Hide resolved
owncloudApp/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.kt Outdated Show resolved Hide resolved
owncloudApp/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.kt Outdated Show resolved Hide resolved
...in/java/com/owncloud/android/data/files/datasources/implementation/OCRemoteFileDataSource.kt Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/FileRepository.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/FileRepository.kt Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/CopyFileUseCase.kt Show resolved Hide resolved
owncloudDomain/src/test/java/com/owncloud/android/domain/files/usecases/CopyFileUseCaseTest.kt Outdated Show resolved Hide resolved
owncloudDomain/src/test/java/com/owncloud/android/domain/files/usecases/MoveFileUseCaseTest.kt Show resolved Hide resolved
JuancaG05
JuancaG05 requested changes Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Second review round here @manuelplazaspalacio! Check the new comment I added and please, check the comments you didn't reply in the previous review round (the ones that are not resolved yet)

owncloudDomain/src/main/java/com/owncloud/android/domain/files/FileRepository.kt Outdated Show resolved Hide resolved
@jesmrec jesmrec mentioned this pull request Jun 13, 2023
Open
manuelplazaspalacio added a commit that referenced this pull request Jun 13, 2023
@manuelplazaspalacio
Change name changelog to #4062.
fa04d6e
@manuelplazaspalacio manuelplazaspalacio force-pushed the feature/copy_move_conflic_solved_users branch 2 times, most recently from 93eb0e3 to b071504 Compare June 13, 2023 15:44
JuancaG05
JuancaG05 approved these changes Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM! Let's move this forward! 🥇 @manuelplazaspalacio

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2023 •
edited
Loading

(1) [FIXED]

In the conflict dialog, File wording appears even when the item is a folder:

Screenshot 2023-06-14 at 10 32 29

In the screenshot, Documents is a folder.

Is there any way to show folder instead of file depending on the item type?

Pixel2, Android11
80144356d

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2023 •
edited
Loading

(2) [FIXED]

  1. In both root folder and in a folder called A , we have 3 files with same name.
  2. Select the 3 files in root
  3. Select as target location the folder A -> 3 conflicts must happen
  4. Fix every conflict with Keep both

Current: only one item is copied with (1) suffix. In network layer, just one single COPY operation for that file. Additionaly, after fixing the first conflict, an snackbar with a message like Index:1 Size: 1 appears (check video below). Not sure if this is a regression since i did not find in the PR's code.

Expected: all conflicting files solved with Keep both are copied including (1)

Video:

device-2023-06-14-111829.mp4

NOTE: Same behaviour if in step 4. you choose Replace

Pixel2, Android11
80144356d

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2023 •
edited
Loading

(3) [improvement] [wont do here]

Check this scenario out:

  1. Select 500 items (or more) to copy or move
  2. Select target folder
  3. Hundreds of conflicts happening in the operation.

User must fix all of them one by one. It's usual that in that case all conflicts will be fixed with the same option.

How feasible/posible is adding a checkbox to the dialog called Apply to all or similar, so that if it's checked all conflicts will be fixed using the selected option once clicked? does it make sense to do in a separate issue? what do you think @manuelplazaspalacio @JuancaG05 ?

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2023

Note for myself: After this is done, we should create another issue to keep same behaviour in uploads. Now, when new item is uploaded and a name conflict happens, file is duplicated with (2) suffix instead of (1).

@manuelplazaspalacio
Solving CR changes.
bb93932
@manuelplazaspalacio
- Changing text on dialog when copying/moving conflict.
1dab1b2 
- Solving bugs copying/moving conflict when doing with two or more files.
@manuelplazaspalacio manuelplazaspalacio force-pushed the feature/copy_move_conflic_solved_users branch from d17f593 to 1dab1b2 Compare June 15, 2023 14:50
@jesmrec
Copy link
Collaborator

jesmrec commented Jun 19, 2023

Approved on my side. Great job!

@jesmrec jesmrec mentioned this pull request Jun 19, 2023
Closed
9 tasks
@manuelplazaspalacio manuelplazaspalacio merged commit 52f7d95 into master Jun 21, 2023
@manuelplazaspalacio manuelplazaspalacio deleted the feature/copy_move_conflic_solved_users branch June 21, 2023 06:34
manuelplazaspalacio added a commit that referenced this pull request Jun 22, 2023
@manuelplazaspalacio
Change name changelog to #4062.
d0e03e8
jesmrec pushed a commit that referenced this pull request Sep 5, 2023
@manuelplazaspalacio @jesmrec
Change name changelog to #4062.
714a3f1
JuancaG05 pushed a commit that referenced this pull request Oct 24, 2023
@manuelplazaspalacio @JuancaG05
Change name changelog to #4062.
a732088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuancaG05 JuancaG05 JuancaG05 approved these changes

Assignees

@manuelplazaspalacio manuelplazaspalacio

Labels
Approved by qa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Copy/move conflict solved by users
3 participants
@manuelplazaspalacio @jesmrec @JuancaG05