Added proper support for multiple OneDrive accounts #3177
Conversation
|
Thanks @AndyMDoyle. I think that, as it is, this will delay showing the fixed drives on the sidebar until all virtual drives are loaded right? In my PR I tried to avoid this by calling |
|
@gave92, I don't have any of my original code in this PR that calls the cloud providers in an async manner. It should function as before. Yes there is a small delay because waiting on OneDriveCloudProvider to get the app service connection will potentially add a delay before ANY cloud drives appear. If it is better that cloud drives appear as soon as they return data, making the cloud providers async and return only their own new drives, and add these to the sidebar instantly may be a better option. |
|
@gave92, I haven't seen any duplicates appearing. Do you have a way to repro that so I can dig in to it? |
As of now everything's fine, no duplicates
It adds a delay before any drive (fixed or cloud appears). It's delaying the fixed drives that annoys me :) In my branch I tried to solve the delay issue (calling DeviceWatcher_EnumerationCompleted to immediately show the fixed drives before enumerating the cloud ones) but this results (for me) in the duplicate issue |
|
@gave92 it sounds like the the fixed drives and cloud drives may need splitting up - or at least using a two different lists and rendering them independently so the fixed drives can happen instantly, followed by the cloud drives once ready? |
|
Current drive loading steps (from Drives.cs -> EnumerateDrives): await GetDrivesAsync(); --> loads fixed drives to `drivesList` (does not add to UI list)
await GetVirtualDrivesListAsync(); --> loads cloud drives to `drivesList` (does not add to UI list)
DeviceWatcher_EnumerationCompleted(null, null); --> updates the UI listTo solve the delay issue I thought it was enough to: await GetDrivesAsync();
DeviceWatcher_EnumerationCompleted(null, null); --> add this to immediately show the fixed drives
await GetVirtualDrivesListAsync();
DeviceWatcher_EnumerationCompleted(null, null); --> add cloud drives to UIThis for unknown reasons results in "visual duplicates": "visual" meaning that backing list has no duplicates but on the UI two drives with the same label appear (sorry I know this is unclear, I'll try to post a screenshot) Point is: I'd like to avoid the delay before showing, at least, the fixed drives, but I don't know how :) Edit: I believe you had a solution for this in your original PR, I'll review that again |
The goal is to display these separately as part of #692, so splitting them up will be a good idea. |
|
@gave92, I have a theory why the duplication is occurring. I'll test it out tomorrow (it's late here). If it is small fix I will add it to this PR. Otherwise maybe this PR should be merged and I can work on the duplication and displaying them independently. I did do part of this while doing that last PR but undid those changes as I didn't want to end up introducing even more changes. |
|
@gave92, @yaichenbaum, how does this look?
No duplication that I have been able to reproduce. Local drives and cloud drives are now handled independently and can each update their own section of the sidebar when changes occur. Drives always appears directly below the list of pinned locations, and Cloud Drives is always tagged on at the end so it is always below the local drives. Obviously it is unrelated to this PR really, so if you like the look of it, I will raise a new PR for this specific change. |
|
I think it looks very good, great work! I have no particular issue doing this here, as it solves the delay problem that this pr would introduce but also a different PR is ok if we merge it before the next public release. Edit: the duplicate issue looks like due to a bug in NavigationView and it's "timing based" so e.g if I comment out loading the cloud drives I'm always getting duplicate fixed drives :/ The issue is not very common and not tied to this PR so we can think about it another time xD |
gave92
added
the
ready to merge
Looks good! Looking forward to your next PR. |
yaira2
changed the title
|
Looks like now it takes 10 sec to load all drives. Note: there is no OneDrive installed on my computer. |
|
@yaichenbaum I didn't test #3191 yet. |
|
@tsvietOK, Those repeated messages are due to Files attempting to connect to, and obtain some information from, the full trust process. Based on feedback from @gave92, FullTrust is given 10 seconds to respond before timing out, with a 200ms delay between each connection attempt. Sometimes the full trust process takes a while to load and initiate which is why the first launch can take longer. I have another update I have been testing that moves the drives and cloud drives enumeration out of SettingsViewModel, and initialises these earlier at startup. Performance seems pretty good, but it will still be at the mercy of FullTrust if that doesn't respond quickly enough. My next attempt is to move these completely to the background so drive and particular cloud drive enumeration runs in the background and populates the UI when finish. Another though I had about performance was that it might be a good idea to list the cloud drive providers in settings, and allow them to be turned on and off individually. This way, if a user has OneDrive setup but doesn't use it, they can prevent any attempt at getting the details and free up space in the UI. Thoughts? |
That's something we can explore, we would need to settle on a design for that so it probably shouldn't be part of this PR, but it's something to look at. |
|
Completely agree. Can't have a PR grow far beyond its aims. |
Related to #3095.
Discussion also took place on PR #3099.
This PR adds support for personal and business OneDrive accounts being loaded from the Windows Registry. This allows all OneDrive accounts to be listed exactly as they are in Windows Explorer, regardless of how may OneDrive accounts the user has setup.