Code Quality: Sync the jump list with Explorer at the startup #17564
Conversation
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
71afbb3 to
9eb7ae9
Compare
0x5bfa
changed the title
There was a problem hiding this comment.
Is this code used at all? If not, we can remove it.
There was a problem hiding this comment.
Yeah used.
There was a problem hiding this comment.
We may need to put something in place to prevent stale jumplists after updating Files or Windows.
There was a problem hiding this comment.
We can do the same as what we do at the startup, clear all and fetch items from Explorer.
Should we do it?
There was a problem hiding this comment.
Does the update task run even if Files is closed? In that case, I think we should do this.
There was a problem hiding this comment.
I think we should make sure to reset our previous jump list too.
| @@ -228,8 +227,6 @@ public async Task SetWorkingDirectoryAsync(string? value) | |||
|
|
|||
| if (value == "Home" || value == "ReleaseNotes" || value == "Settings") | |||
| currentStorageFolder = null; | |||
| else | |||
| _ = Task.Run(() => jumpListService.AddFolderAsync(value)); | |||
There was a problem hiding this comment.
Does this need a replaement from our end?
There was a problem hiding this comment.
I think that if we don't manually add folders when opening them in Files, they won't get added to the frequent access list.
There was a problem hiding this comment.
Ah right, but do we really want to add a folder just because you browsed it? In Explorer one gets added only when you use one of some verbs (open, for example) on a file. If you just keep clicking recursively, then all of them get added, it may be excessive.
There was a problem hiding this comment.
In my opinion, yes. It's more useful when recently opened folders are actually displayed.
There was a problem hiding this comment.
That case, to keep the synchronization, we should update Explorer's jump list, then the watcher (I will create in the next PR) would notice it and update Files' one too.
What you think?
There was a problem hiding this comment.
Sounds good. That said, I would prefer it if the watcher was included in this PR as well so that we don't have any gap in functionality.
There was a problem hiding this comment.
Oh I totally thought I should separate into smaller PRs for easier review. But it's just adding a watcher class so should be easy to review either way. I'll implement it in this PR today.
yaira2
force-pushed
the
main
branch
2 times, most recently
from
97999e5 to
806f922
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
9306256 to
c773727
Compare
Resolved / Related Issues
This PR only duplicates the Explorer's jump list and doesn't watch changes to it to sync while Files is running. It'll be implemented in the next PR.
Steps used to test these changes