Improve drive detection and UI sync to deal with duplicate entries

[AndyMDoyle]

#3231

Primary change is making the app service connection a singleton which means multiple messages can be sent without issues launching additional processes. A lot of this has been taken from a @gave92's network drives PR and should make that PR a smaller update.

The logic to add local drives and cloud drives to the side bar has also been refactored to ensure this is done on the UI thread and that a lock is obtained to prevent multiple updates to the side bar items at the same time. Now a lot of the code is async, this is done using a SemaphoreSlim to ensure one thread at a time updates the side bar items.

So far this update has been 100% reliable in debug mode, release mode, and with SharePoint sync folder support on top which results in an additional concurrent full trust call at startup.

[gave92]

Still duplicates :(

Wrong drive order (I believe we want to keep it sorted by letter, not label):

[AndyMDoyle]

@gave92, PR updated to remove drive ordering. I've realised I didn't see that because my drives are named in a way they end up in order of the drive letters.

Can you try and repro the duplication issue by swapping

_ = Task.Factory.StartNew(async () =>
{
    await DrivesManager.EnumerateDrivesAsync();
    await CloudDrivesManager.EnumerateDrivesAsync();
});

for

await DrivesManager.EnumerateDrivesAsync();
await CloudDrivesManager.EnumerateDrivesAsync();

Task.Factory.StartNew doesn't preserve the synchronization context and has caused me issues in the past. This way the drive lists should be fully populated before MainPage is constructed.

[gave92]

@AndyMDoyle yeah but I really don't want to wait for the sidebar to be fully populated before showing MainPage :)
I'm pushing here what fixes the duplicate issue for me, that is calling a Collection.Reset after loading items (note: there's something wrong with the new BulkObservableCollection, app crashes after minimize/restore)

[AndyMDoyle]

@gave92, neither do I really but doing that did get rid of duplication issue for me.

Your commit looks great and I can't see any issues on my side. One question though, the existing usage of BulkConcurrentObservableCollection in ItemViewModel calls the BeginBulkOperation() method before working on the collection, followed by EndBulkOperation(). Should BeginBulkOperation() be called from these new places?

And in ItemViewModel(), EndBulkOperation() is called on the UI thread, so is that also needed instead of that happening on a background thread?

[gave92]

Yes, EndBulkOperation() must be called on the UI thread, I should be already doing it, if not please correct :)
BeginBulkOperation() is not really mandatory in this case. It's used to pause firing events for collection changes but here I just want to call an additional NotifyCollectionChangedAction.Reset event when I'm done adding items to workaround the duplicate issue.

Your commit looks great and I can't see any issues on my side

I can't believe that xD If you minimize and restore the app without being attached to the debugger are you really not getting a crash?

Edit: latest commit fixes the crash after minimize/restore. I'm still interested in knowing whether before this commit you were getting a crash as well

[AndyMDoyle]

@gave92, yes I am seeing a crash related to BulkConcurrentObservableCollection.OnCollectionChanged if I run without the debugger.

Trying the latest commit now.

[gave92]

Hi @hez2010 we need your help :) In this PR we were trying to switch the MainPage SidebarItems list from ObservableCollection to the BulkConcurrentObservableCollection. Unfortunately we were getting a crash anytime the Remove method was called with the following error: Parameter name: index|System.ArgumentOutOfRangeException: This collection cannot work with indices larger than Int32.MaxValue - 1 (0x7FFFFFFF - 1).. The difference from ItemViewModel is that we never call BeginBulkOperation but use BulkConcurrentObservableCollection as a "standard" ObservableCollection.

Do you understand why the app is crashing? If you wish to test you can checkout commit 5eceb15 (not the latest one). To trigger the bug you can suspend/resume the app (or minimize/restore it with no debugger attached).

[AndyMDoyle]

@gave92 everything looks okay when using an ObservableCollection. Release mode starts up nice an quick and the local drives and cloud drives sections appear when ready.

[hez2010]

@gave92 It's caused by a mistake that passing Remove event argument without index. Also it seems that the SideBar underlaying control needs a PropertyChanged to Items[], I've added this.

@AndyMDoyle I've proposed a fix to BulkConcurrentObservableCollection, could I push the fix to your branch (add me as collaborator in your forked repo)? Or you can merge this branch to yours: https://github.com/hez2010/files-uwp/tree/3231_drive_sync_and_duplicates

[hez2010]

I think this PR is ready for merge. @gave92 could you test again to see if there're still issues remaining or performance degradation about BulkConcurrentObservableCollection?

[yaira2]

@AndyMDoyle Great work!