Skip to content

Conversation

lukeblevins
Copy link
Contributor

This requires extensive testing as it may have introduced a few bugs.

@lukeblevins lukeblevins requested review from tsvietOK and yaira2 June 9, 2020 02:37
@lukeblevins lukeblevins changed the title Implement filesystem watching via ReadDirectoryChangesW TESTING: Implement filesystem watching via ReadDirectoryChangesW Jun 9, 2020
@ghost ghost added the needs - code review label Jun 9, 2020
@tsvietOK
Copy link
Contributor

tsvietOK commented Jun 9, 2020

Random crash when navigating to a folder still exists and VS can't catch an exception.

Files and folders are not sorted.
image

After folder creation or renaming there 3 or more folders displaying, fixes by folder refresh. For files the same.
Rename_1

@lukeblevins lukeblevins changed the title TESTING: Implement filesystem watching via ReadDirectoryChangesW WIP: Implement filesystem watching via ReadDirectoryChangesW Jun 9, 2020
@lukeblevins
Copy link
Contributor Author

Thanks for the feedback everybody. I'll make it a priority to fix these. Keep up the testing!

@gave92
Copy link
Member

gave92 commented Jun 9, 2020

@duke7553 Just a curiosity: There is the FileSystemWatcher class which I think is supported in UWP when you use the desktop bridge. It's basically a convenience class that calls ReadDirectoryChagesW internally. Have you considered using this instead of doing the stuff "low level"?

@lukeblevins
Copy link
Contributor Author

@gave92 Personally, I'm trying to keep as much core functionality as possible outside the Full Trust component. This is because FullTrust components are not supported in Windows 10X.

@gave92
Copy link
Member

gave92 commented Jun 9, 2020

I meant: I think that FileSystemWatcher is supported in UWP. FileSystemWatcher is part of netstandard 2.0 and uwp supports that.

@lukeblevins
Copy link
Contributor Author

@gave92 Good thought. Unfortunately, .NET Classes are unable to access brokered filesystem locations which WinRT apps use, which means we have to do it the native way.

@gave92
Copy link
Member

gave92 commented Jun 9, 2020

@duke7553 thanks for the clarification. I thought that the broadFileSystemAccess capability was enough to bypass this.

@lukeblevins
Copy link
Contributor Author

@gave92 @tsvietOK If someone could help me understand why there are still duplicate items (after adding one item) I'd be grateful. In my testing, it occurs after quickly refreshing multiple times.

@lukeblevins
Copy link
Contributor Author

It is clearly because the method is fired twice, but how to prevent that, IDK

@gave92
Copy link
Member

gave92 commented Jun 10, 2020

@duke7553 I still have to review the code but I had similar issue with the recycle bin functionality: multiple filesystem update events where firing at the same time and I was ending with duplicate items.

I solved it using a semaphore so the refresh function can only be entered after the previous call has ended.

@tsvietOK
Copy link
Contributor

@duke7553 Sometimes displayed(after creation) 4 or more folders/files instead of 1. In my testing this bug is not appears when refreshing items.

@lukeblevins
Copy link
Contributor Author

@tsvietOK In my testing, it was almost like refreshing multiple times didn't properly cancel the previous watcher.

@lukeblevins
Copy link
Contributor Author

Refreshing causes the next item add/removal action to be duplicated.

@gave92
Copy link
Member

gave92 commented Jun 10, 2020

@duke7553 very stupid suggestion: you are not awaiting the call to AddFileOrFolder

@lukeblevins lukeblevins changed the title WIP: Implement filesystem watching via ReadDirectoryChangesW Implement filesystem watching via ReadDirectoryChangesW Jun 10, 2020
@lukeblevins
Copy link
Contributor Author

@yaichenbaum @tsvietOK @gave92 This is ready for review. If at all possible, we should review this one quickly before the next release.

@lukeblevins lukeblevins requested a review from tsvietOK June 10, 2020 22:07
@yaira2
Copy link
Member

yaira2 commented Jun 10, 2020

:shipit:

@yaira2 yaira2 merged commit 7c241c9 into master Jun 10, 2020
@gave92
Copy link
Member

gave92 commented Jun 11, 2020

@duke7553 refreshing + renaming still has some issues for me:

2020-06-11 07:47:45.2148|ERROR|Files.App|Collection was modified; enumeration operation may not execute.|System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
at System.Collections.Generic.List1.Enumerator.MoveNextRare() at System.Collections.Generic.List1.Enumerator.MoveNext()
at System.Linq.Enumerable.SelectIListIterator2.MoveNext() at System.Linq.Enumerable.Contains[TSource](IEnumerable1 source, TSource value, IEqualityComparer1 comparer) at System.Linq.Enumerable.Contains[TSource](IEnumerable1 source, TSource value)
at Files.Filesystem.ItemViewModel.<>c__DisplayClass72_0.b__0(IAsyncAction x)
--- End of stack trace from previous location where exception was thrown ---
at Files.Filesystem.ItemViewModel.WatchForDirectoryChanges(String path)
at System.Runtime.CompilerServices.AsyncMethodBuilderCore.<>c.b__7_0(Object state)
at System.Threading.WinRTSynchronizationContextBase.Invoker.InvokeCore()

@yaira2 yaira2 deleted the watcher-fixes branch June 25, 2020 02:01
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants