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

Add option to notify when synced folder overcomes set size limit #5868

Merged
merged 57 commits into from
Aug 4, 2023

Conversation

claucambra
Copy link
Collaborator

This expands upon the existing feature to not sync new folders larger than a set limit

Closes #5818

Screenshot 2023-07-05 at 23 40 28

Screenshot 2023-07-06 at 14 46 01

@claucambra claucambra self-assigned this Jul 6, 2023
@claucambra claucambra force-pushed the work/folder-threshold-improve branch from 062960f to be6b0bb Compare July 6, 2023 07:06
@mgallien
Copy link
Collaborator

mgallien commented Jul 7, 2023

@claucambra build is failing

@claucambra
Copy link
Collaborator Author

@claucambra build is failing

Now fixed

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/folderwatcher_win.h Show resolved Hide resolved
@tobiasKaminsky
Copy link
Member

@claucambra I gave this a test run

  • enabled above option, limit to 50Mb
  • create folder on server
  • added 20kb
  • synced
  • added 54Mb
    --> file was downloaded without problems

It is shown here:
image

@claucambra
Copy link
Collaborator Author

@claucambra I gave this a test run

* enabled above option, limit to 50Mb

* create folder on server

* added 20kb

* synced

* added 54Mb
  --> file was downloaded without problems

It is shown here: image

Great, working as expected then :) did you get the notification?

@tobiasKaminsky
Copy link
Member

No, no notification.
And it should not sync folder, but first ask what to do, like in your first screenshot:

  • client notices folder about limit
  • client pauses sync for now
  • client asks user what to do
  • "keep syncing" -> resume sync
  • "stop syncing" -> remove folder locally

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

@claucambra please add automated tests covering teh changes in libsync code (mainly SyncEngine and related classes

@claucambra
Copy link
Collaborator Author

claucambra commented Jul 17, 2023

No, no notification. And it should not sync folder, but first ask what to do, like in your first screenshot:

* client notices folder about limit

* client pauses sync for now

* client asks user what to do

* "keep syncing" -> resume sync

* "stop syncing" -> remove folder locally

As far as I understood the task is to warn the user, not stop syncing the folder/stop the current sync run -- I imagine this would be annoying if sync suddenly stopped when folders grow, no?

Could you also send client logs for the sync run to debug the lack of a notification? Is there anything in the activity list? Thanks!

@tobiasKaminsky
Copy link
Member

As the other options are also stated "ask for confirmation" I think we should do the same here.
I am sorry that I was not clear enough when writing description.

Once you have changed it, I can re-test and send debug archive if needed.

@claucambra
Copy link
Collaborator Author

As the other options are also stated "ask for confirmation" I think we should do the same here. I am sorry that I was not clear enough when writing description.

Once you have changed it, I can re-test and send debug archive if needed.

I have added an additional checkbox to disable synchronisation of files that have overcome limit now, feel free to test :)

@claucambra claucambra force-pushed the work/folder-threshold-improve branch from eb56ca0 to c5d0a8d Compare July 24, 2023 09:25
@tobiasKaminsky
Copy link
Member

Works great!
Ready for merge :-)

src/gui/folder.cpp Outdated Show resolved Hide resolved
src/gui/folder.cpp Outdated Show resolved Hide resolved
src/gui/folder.cpp Show resolved Hide resolved
src/gui/folderman.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/libsync/discoveryphase.cpp Show resolved Hide resolved
@claucambra claucambra force-pushed the work/folder-threshold-improve branch 2 times, most recently from 350df23 to 30a574e Compare August 3, 2023 07:50
@claucambra claucambra force-pushed the work/folder-threshold-improve branch 2 times, most recently from feb2749 to fc45ef7 Compare August 4, 2023 03:58
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #5868 (fc45ef7) into master (8c65b06) will increase coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 79.16%.

❗ Current head fc45ef7 differs from pull request most recent head a5d7c75. Consider uploading reports for the commit a5d7c75 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5868      +/-   ##
==========================================
+ Coverage   60.06%   60.08%   +0.02%     
==========================================
  Files         145      145              
  Lines       18771    18797      +26     
==========================================
+ Hits        11275    11295      +20     
- Misses       7496     7502       +6     
Files Changed Coverage Δ
src/common/syncjournaldb.h 75.00% <ø> (ø)
src/common/utility.h 0.00% <ø> (ø)
src/libsync/discovery.h 59.25% <ø> (ø)
src/libsync/discoveryphase.h 20.00% <ø> (ø)
src/libsync/logger.cpp 32.96% <ø> (+0.35%) ⬆️
src/libsync/logger.h 61.53% <ø> (ø)
src/libsync/syncengine.h 65.00% <ø> (ø)
src/libsync/theme.cpp 19.13% <0.00%> (+0.07%) ⬆️
src/libsync/configfile.cpp 27.08% <50.00%> (+0.76%) ⬆️
src/libsync/discovery.cpp 86.54% <76.74%> (+0.21%) ⬆️
... and 5 more

... and 1 file with indirect coverage changes

…hat have now overcome limit

Signed-off-by: Claudio Cambra <[email protected]>
…d disabling sync of folders that become big

Signed-off-by: Claudio Cambra <[email protected]>
…r already black or white listed

Signed-off-by: Claudio Cambra <[email protected]>
…n activitydata, refer to this in usage

Signed-off-by: Claudio Cambra <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

36.3% 36.3% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.14.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@claucambra claucambra merged commit a89fe84 into master Aug 4, 2023
11 of 12 checks passed
@claucambra claucambra deleted the work/folder-threshold-improve branch August 4, 2023 10:29
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5868-a5d7c75e6f9e0e385b86780090c5c0c39e7bd54e-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.

@mgallien mgallien added this to the 3.10.0 milestone Sep 7, 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.

Warn if folder during sync overcomes configured threshold
6 participants