-
Notifications
You must be signed in to change notification settings - Fork 793
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
Set VFS PinState to Excluded for ignored files. #5853
Set VFS PinState to Excluded for ignored files. #5853
Conversation
54de074
to
fed55a1
Compare
@mgallien Is this PR eligible to be merged? Should someone else review this PR? Please do not hesitate to contact me if you need more information or something should be changed prior to being merged. |
@m7913d sorry for the delayed review |
fed55a1
to
ae2ce45
Compare
Note that the "Windows Build and Test / Build" workflow fails due to external changes. PR #5874 provides a fix for this failed workflow. Please rerun the check after that PR is approved. |
ae2ce45
to
e1d7804
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5853 +/- ##
==========================================
- Coverage 60.06% 59.88% -0.18%
==========================================
Files 145 145
Lines 18746 18756 +10
==========================================
- Hits 11259 11232 -27
- Misses 7487 7524 +37
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small coding improvements
f56ae85
to
6fdf0f0
Compare
6fdf0f0
to
6262336
Compare
@m7913d could you squash your commits as I do not think it is needed to keep 3 commits ? |
/backport to stable-3.9 |
6262336
to
af8a885
Compare
@mgallien Commits are squashed. |
@mgallien @claucambra As mentioned in my second comment, this PR generates a lot of warnings "Couldn't find pin state for regular non-placeholder file". Is this ok for you? Should we remove the warning, lower the warning to a debug/info message or use another function to check if the file has a pin state? |
@m7913d I think (but I cannot check) that this warning dates from the early stages of this code |
7baa0bc
to
793c99b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment
somehow missed that earlier, sorry
867d468
to
4bd4c02
Compare
Setting PinState to Excluded ensures the syncing icon is not shown for ignored items. If the PinState is not set to Excluded, also all parent directories are shown as being synced, which is very inconvenient for the end user as it seems that some folder are never fully synced by Nextcloud which isn't the case. As long as .lnk files are not converted to placeholder files, also set them to Excluded to hide the syncing icon. Closes nextcloud#5524 Closes nextcloud#5594 Co-authored-by: Matthieu Gallien <[email protected]> Signed-off-by: Dries Mys <[email protected]>
F.ex. Folder::slotWatchedPathChanged calls this function on non- placeholder files to check if the ignored file is already assigned an Excluded pin state. Note that it is allowed to set the pin state to Excluded on non-placeholder files. Signed-off-by: Dries Mys <[email protected]>
Signed-off-by: Dries Mys <[email protected]>
4bd4c02
to
55f27cd
Compare
AppImage file: nextcloud-PR-5853-55f27cdb9657eb53aed0f87f533ff1cd1611d98f-x86_64.AppImage |
Setting PinState to Excluded ensures the syncing icon is not shown for ignored items.
If the PinState is not set to Excluded, also all parent directories are shown as being synced, which is very inconvenient for the end user as it seems that some folder are never fully synced by Nextcloud which isn't the case.
As long as .lnk files are not converted to placeholder files, also set them to Excluded to hide the syncing icon.
Closes #5524
Closes #5594
Closes #5825