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

Feature/resolve all conflicts #5635

Merged
merged 28 commits into from
May 17, 2023
Merged

Feature/resolve all conflicts #5635

merged 28 commits into from
May 17, 2023

Conversation

mgallien
Copy link
Collaborator

implementation of a dialog to resolve conflicts as a batch

will allow solving all conflicts at once

FIX #2786

@mgallien
Copy link
Collaborator Author

mgallien commented Apr 28, 2023

Current state of the dialog

image

@nimishavijay
@jancborchardt
feel free to provide feedback (current implementation is not yet functional for you to test as it is using mockup data)
I will fix the title of the dialog
next step would be to make it a drawer from main dialog like the share UI and file activity UI
there is also an issue due to scaling on my environment making the snapshot appear very big
it currently uses the same font size that is used by the server web UI for conflicts when uploading several files

I suggest using: "Solve synchronization conflicts" for the dialog title

@nimishavijay
Copy link
Member

nimishavijay commented Apr 28, 2023

Looks good! Great functionality :)

I will fix the title of the dialog
I suggest using: "Solve synchronization conflicts" for the dialog title

Yep, great suggestion! To keep it simple, we can shorten it to "Solve sync conflicts"

I have suggested a few changes with the appearance, let us know what is feasible :) The general idea is for it to look like this:

image

  • Change the wording
    • "Which files do you want to keep?" --> "Choose if you want to keep the local version, server version, or both."
    • "Local version""Server version" --> "All local versions" "All server versions"
  • Use bold text (if not possible, regular text) for the name of the file as it is the most important info for choosing.
  • Place checkboxes to the left of the file icon, following the pattern of checkboxes that is used everywhere.
  • Use a smaller file icon to save space
  • Add wording "Local version" or "Server version" on the first line for screenreaders, and also easy scannability

What do you think? also cc @jancborchardt for any more feedback :)

@mgallien
Copy link
Collaborator Author

mgallien commented Apr 28, 2023

@nimishavijay thanks a lot for the feedback
see new screenshot below
image

I noticed an issue with the wrong spacing between preview and text in the second column
I will fix it and update the screenshot

@mgallien mgallien force-pushed the feature/resolveAllConflicts branch from 8e986af to 9eeae0b Compare April 28, 2023 13:35
@mgallien
Copy link
Collaborator Author

image

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #5635 (6c7f810) into master (37bd3f3) will increase coverage by 0.95%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5635      +/-   ##
==========================================
+ Coverage   59.38%   60.33%   +0.95%     
==========================================
  Files         143      143              
  Lines       18524    18524              
==========================================
+ Hits        11000    11176     +176     
+ Misses       7524     7348     -176     

see 13 files with indirect coverage changes

@nimishavijay
Copy link
Member

nimishavijay commented Apr 28, 2023

That's great! 2 more tiny changes:

  • "Choose if you want to keep the local version, server version, or both." ( ? --> . use full stop instead of question mark)
  • If possible, reduce the spacing between the 3 lines in the file info by 2 or 3 px

Other than that it looks great! :)

@mgallien
Copy link
Collaborator Author

mgallien commented May 2, 2023

image

@nimishavijay thanks for the feedback
see the screenshot on top for the latest state
I have another question
I am unsure how best show up the action to solve all conflicts
should I add a drop down menu to access this from a long click on the "Resolve conflict" button (see
image
)

@mgallien mgallien force-pushed the feature/resolveAllConflicts branch from 9eeae0b to ce1b886 Compare May 2, 2023 12:54
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/gui/syncconflictsmodel.h Outdated Show resolved Hide resolved
src/gui/syncconflictsmodel.h Outdated Show resolved Hide resolved
src/gui/syncconflictsmodel.h Outdated Show resolved Hide resolved
src/gui/syncconflictsmodel.h Outdated Show resolved Hide resolved
@mgallien mgallien force-pushed the feature/resolveAllConflicts branch from ce1b886 to bf2266a Compare May 3, 2023 08:15
@github-actions github-actions bot dismissed their stale review May 3, 2023 08:27

No clang-tidy warnings found so I assume my comments were addressed

@mgallien mgallien force-pushed the feature/resolveAllConflicts branch 3 times, most recently from a9e1b80 to dec4601 Compare May 3, 2023 10:12
@jancborchardt
Copy link
Member

@mgallien a detail for the button naming: Instead of the generic "Ok", can we call it "Resolve conflicts"?

@mgallien mgallien force-pushed the feature/resolveAllConflicts branch from dec4601 to 15047b7 Compare May 3, 2023 12:00
@mgallien
Copy link
Collaborator Author

mgallien commented May 3, 2023

@mgallien a detail for the button naming: Instead of the generic "Ok", can we call it "Resolve conflicts"?

@jancborchardt no problem

image

@nimishavijay
Copy link
Member

see the screenshot on top for the latest state

Look good! We could add around 4px more space below each row of files so that each section is a bit more spaced out, other than that it is good to go! 🚀

should I add a drop down menu to access this from a long click on the "Resolve conflict"

Would this be in the tray menu? Long clicks are generally not an easily discoverable way of interacting in desktop, it's more suitable for mobile :) I would say a visible button next to this button would be better. Perhaps this was also already discussed in the design review call today with @jancborchardt :)

@mgallien mgallien force-pushed the feature/resolveAllConflicts branch 2 times, most recently from d7a9871 to 430ac93 Compare May 4, 2023 07:37
@mgallien
Copy link
Collaborator Author

mgallien commented May 4, 2023

@nimishavijay @jancborchardt

see the screenshot on top for the latest state

Look good! We could add around 4px more space below each row of files so that each section is a bit more spaced out, other than that it is good to go! rocket

done
see screenshot below

should I add a drop down menu to access this from a long click on the "Resolve conflict"

Would this be in the tray menu? Long clicks are generally not an easily discoverable way of interacting in desktop, it's more suitable for mobile :) I would say a visible button next to this button would be better. Perhaps this was also already discussed in the design review call today with @jancborchardt :)

I followed the suggestion from Tobias to replace "Sync now" by "Solve all conflicts" when there are many conflicts (> 3)
see

image

image

@nimishavijay
Copy link
Member

nimishavijay commented May 4, 2023

done
see screenshot below

Looks great now!

I followed the suggestion from Tobias to replace "Sync now" by "Solve all conflicts" when there are many conflicts (> 3)
see

This sounds good to me as well :) Some suggestions:

  • "Solve all conflicts" --> "Resolve conflicts" change the wording to make it consistent
  • I would also say that we can show it when there is at least one file conflict, not just for the case when there are many. That way there could be one consistent point of entry for resolving conflicts

@mgallien
Copy link
Collaborator Author

mgallien commented May 4, 2023

@nimishavijay thanks for the feedback
all done with latest push

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Then design wise it's ready to go! :) 🚀

mgallien added 21 commits May 17, 2023 08:43
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
use more automatyed memory management to reduce possible errors

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien force-pushed the feature/resolveAllConflicts branch from 9998cc7 to 6c7f810 Compare May 17, 2023 06:43
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5635-6c7f81009523866f6a6e912bfb0434626739b74e-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 22 Code Smells

34.7% 34.7% Coverage
0.0% 0.0% Duplication

@mgallien mgallien merged commit d7d52a1 into master May 17, 2023
@mgallien mgallien deleted the feature/resolveAllConflicts branch May 17, 2023 08:35
@steel4me
Copy link

steel4me commented May 17, 2023

Thank you, i've waited years for this. Don't know why but after Migration von NC20 to 25 AiO my VSCode Portable have 250 conflicts in vscode/data/user-data/*. It feels strange, i think other files are not synced when conflicts are not solved. Anyway, this is not related to this issue im sorry.

Downloaded actual stable master .msi and don't get the feature. Is this in cause auf sonarcloud build failed for your merge into master? Is this only Linux? Is it possible for me to get a .msi from a feature branch?

A nice feature for this would be to add a third option "Automatic Solve". In my case i prefer always the "Newest File Version". Nextcloud already "Bolds" the newer Version of the File.

image

@kleajmp
Copy link

kleajmp commented Jun 22, 2023

is this a bug or am I wrong?
running desktop client v3.9.0 on openSuse

first of all I am super happy that the resolve all conflicts dialogue implementation is finally released.
it works for me when I check "All server versions" to keep all newest versions of all files.

but here is the bug:

  • when I really check the dates next to the individual files the server version column shows the older date (actually the exact date of the local version) so these two columns are faulty reversed (!)
  • when I select "keep local version" the newest file is overwritten by the older local copy
  • when select "keep server version" a new set of dialogs appear to confirm if I want to delete the older conflicted copy, a button "yes to all" would be a great addition.

so the thinks to do in my advice:

  • check the date info which are in my case (both on windows as opensuse client) shown in the wrong (opposite) column
  • add a "yes to all" button in the "confirm deletion dialogue" after choosing to keep al the newest versions (server version)

here some images to illustrate the bug and the suggested button addition:
Screenshot_20230622_194304
Screenshot_20230622_194556
Screenshot_20230622_195223
Screenshot_20230622_195512

@mgallien mgallien added this to the 3.9.0 milestone Jul 4, 2023
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.

UI to resolve all conflicts en masse
7 participants