Set VFS PinState to Excluded for ignored files. #5853

m7913d · 2023-07-02T17:19:41Z

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

m7913d · 2023-07-02T17:38:21Z

Note that this implementation generates many warnings "Couldn't find pin state for regular non-placeholder file" when calling VfsCfApi::pinStateLocal inside Folder::slotWatchedPathChanged on non placeholder files. Not sure if this warning is relevant. Note that we could set the PinState on non placeholder files, but querying the PinState seems to fail. OneDrive seems to do the same.
When setting the PinState to Excluded no sync icon is shown for that file. However, on OneDrive a special 'excluded' icon is shown:

I'm not sure if and how we can set such an icon too, but showing no icon is at least much better than showing the syncing icon indefinitely.

m7913d · 2023-07-05T18:24:30Z

@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.

mgallien · 2023-07-07T12:46:27Z

@m7913d sorry for the delayed review
thanks a lot for your PR
I added people to the list of reviewers
should help with getting timely review
cannot promise when I will be able to do it myself

m7913d · 2023-07-09T13:31:58Z

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.

mgallien

thanks a lot

src/gui/folder.cpp

src/gui/folder.h

src/gui/folderwatcher.h

src/libsync/vfs/cfapi/vfs_cfapi.h

codecov · 2023-07-11T19:31:04Z

Codecov Report

Merging #5853 (55f27cd) into master (7524853) will decrease coverage by 0.18%.
The diff coverage is 69.23%.

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

Impacted Files	Coverage Δ
src/common/pinstate.h	`0.00% <ø> (ø)`
src/libsync/vfs/cfapi/cfapiwrapper.cpp	`73.07% <50.00%> (+0.12%)`	⬆️
src/libsync/vfs/cfapi/vfs_cfapi.cpp	`82.44% <72.72%> (-0.63%)`	⬇️

... and 6 files with indirect coverage changes

src/libsync/vfs/cfapi/vfs_cfapi.cpp

src/gui/folderwatcher.cpp

src/gui/folder.cpp

m7913d

Small coding improvements

src/libsync/vfs/cfapi/vfs_cfapi.cpp

src/gui/folder.cpp

mgallien · 2023-07-17T07:46:39Z

@m7913d could you squash your commits as I do not think it is needed to keep 3 commits ?
then I approve and make sure this is merged
thanks again for your contribution

mgallien · 2023-07-17T08:07:11Z

/backport to stable-3.9

m7913d · 2023-07-17T08:40:11Z

@mgallien Commits are squashed.

m7913d · 2023-07-17T12:47:01Z

@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?

mgallien · 2023-07-18T15:09:31Z

@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
I would go with debug level as I would not expect this warning to really be useful now
thanks for pinging us about this as I had overlooked your question

src/libsync/vfs/cfapi/vfs_cfapi.cpp

mgallien

see my comment
somehow missed that earlier, sorry

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]>

nextcloud-desktop-bot · 2023-07-20T12:43:55Z

AppImage file: nextcloud-PR-5853-55f27cdb9657eb53aed0f87f533ff1cd1611d98f-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.

m7913d force-pushed the ignoredFilesExcludedPinState branch 2 times, most recently from 54de074 to fed55a1 Compare July 2, 2023 17:49

PhilippSchlesinger mentioned this pull request Jul 7, 2023

[Bug]: Virtual files - Status icons from shortcuts don't get updated #5825

Closed

8 tasks

mgallien requested review from mgallien, allexzander, camilasan and claucambra July 7, 2023 12:45

mgallien force-pushed the ignoredFilesExcludedPinState branch from fed55a1 to ae2ce45 Compare July 7, 2023 12:47

mgallien force-pushed the ignoredFilesExcludedPinState branch from ae2ce45 to e1d7804 Compare July 11, 2023 16:03

mgallien requested changes Jul 11, 2023

View reviewed changes

src/gui/folder.cpp Outdated Show resolved Hide resolved

src/gui/folder.h Outdated Show resolved Hide resolved

src/gui/folderwatcher.h Outdated Show resolved Hide resolved

src/libsync/vfs/cfapi/vfs_cfapi.h Outdated Show resolved Hide resolved

m7913d commented Jul 11, 2023

View reviewed changes

src/libsync/vfs/cfapi/vfs_cfapi.cpp Outdated Show resolved Hide resolved

m7913d commented Jul 11, 2023

View reviewed changes

src/gui/folderwatcher.cpp Outdated Show resolved Hide resolved

m7913d commented Jul 11, 2023

View reviewed changes

src/gui/folder.cpp Outdated Show resolved Hide resolved

m7913d commented Jul 11, 2023

View reviewed changes

mgallien requested changes Jul 13, 2023

View reviewed changes

src/libsync/vfs/cfapi/vfs_cfapi.cpp Outdated Show resolved Hide resolved

src/gui/folder.cpp Outdated Show resolved Hide resolved

src/gui/folder.cpp Outdated Show resolved Hide resolved

src/gui/folder.cpp Outdated Show resolved Hide resolved

m7913d force-pushed the ignoredFilesExcludedPinState branch from f56ae85 to 6fdf0f0 Compare July 14, 2023 08:12

m7913d requested a review from mgallien July 14, 2023 08:40

mgallien force-pushed the ignoredFilesExcludedPinState branch from 6fdf0f0 to 6262336 Compare July 17, 2023 07:45

backportbot-nextcloud bot added the backport-request label Jul 17, 2023

m7913d force-pushed the ignoredFilesExcludedPinState branch from 6262336 to af8a885 Compare July 17, 2023 08:37

mgallien approved these changes Jul 17, 2023

View reviewed changes

claucambra approved these changes Jul 17, 2023

View reviewed changes

m7913d force-pushed the ignoredFilesExcludedPinState branch 4 times, most recently from 7baa0bc to 793c99b Compare July 18, 2023 21:32

mgallien reviewed Jul 18, 2023

View reviewed changes

src/libsync/vfs/cfapi/vfs_cfapi.cpp Outdated Show resolved Hide resolved

mgallien requested changes Jul 18, 2023

View reviewed changes

m7913d force-pushed the ignoredFilesExcludedPinState branch 2 times, most recently from 867d468 to 4bd4c02 Compare July 19, 2023 09:47

m7913d requested a review from mgallien July 19, 2023 09:49

mgallien approved these changes Jul 20, 2023

View reviewed changes

mgallien enabled auto-merge July 20, 2023 11:44

m7913d and others added 3 commits July 20, 2023 13:44

Small coding improvements

55f27cd

Signed-off-by: Dries Mys <[email protected]>

mgallien force-pushed the ignoredFilesExcludedPinState branch from 4bd4c02 to 55f27cd Compare July 20, 2023 11:44

mgallien disabled auto-merge July 20, 2023 13:01

mgallien merged commit 759c2a2 into nextcloud:master Jul 20, 2023
9 of 10 checks passed

backportbot-nextcloud bot mentioned this pull request Jul 20, 2023

[stable-3.9] Set VFS PinState to Excluded for ignored files. #5904

Merged

mgallien added this to the 3.10.0 milestone Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set VFS PinState to Excluded for ignored files. #5853

Set VFS PinState to Excluded for ignored files. #5853

m7913d commented Jul 2, 2023 •

edited

Loading

m7913d commented Jul 2, 2023

m7913d commented Jul 5, 2023

mgallien commented Jul 7, 2023

m7913d commented Jul 9, 2023

mgallien left a comment

codecov bot commented Jul 11, 2023 •

edited

Loading

m7913d left a comment

mgallien commented Jul 17, 2023

mgallien commented Jul 17, 2023

m7913d commented Jul 17, 2023

m7913d commented Jul 17, 2023

mgallien commented Jul 18, 2023

mgallien left a comment

nextcloud-desktop-bot commented Jul 20, 2023

Set VFS PinState to Excluded for ignored files. #5853

Set VFS PinState to Excluded for ignored files. #5853

Conversation

m7913d commented Jul 2, 2023 • edited Loading

m7913d commented Jul 2, 2023

m7913d commented Jul 5, 2023

mgallien commented Jul 7, 2023

m7913d commented Jul 9, 2023

mgallien left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 11, 2023 • edited Loading

Codecov Report

m7913d left a comment

Choose a reason for hiding this comment

mgallien commented Jul 17, 2023

mgallien commented Jul 17, 2023

m7913d commented Jul 17, 2023

m7913d commented Jul 17, 2023

mgallien commented Jul 18, 2023

mgallien left a comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Jul 20, 2023

m7913d commented Jul 2, 2023 •

edited

Loading

codecov bot commented Jul 11, 2023 •

edited

Loading