Code Quality: Sync the jump list with Explorer #17564
Open
+1,312
−482
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
71afbb3 to
9eb7ae9
Compare
0x5bfa
changed the title
yaira2
reviewed
Sep 7, 2025
yaira2
reviewed
Sep 7, 2025
yaira2
reviewed
Sep 7, 2025
yaira2
reviewed
Sep 7, 2025
yaira2
reviewed
Sep 7, 2025
yaira2
force-pushed
the
main
branch
2 times, most recently
from
97999e5 to
806f922
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
3 times, most recently
from
278a971 to
33efe98
Compare
0x5bfa
changed the title
Member
|
This might help with #14526 |
Member
|
Would this fix #17659 |
Member
Member
Author
|
Yes. This is one of the motivation. (I updated the PR to include this) |
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
a038857 to
3669604
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
9daefd6 to
032a523
Compare
yaira2
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
7529df0 to
ec45efa
Compare
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
2 times, most recently
from
edbda5e to
816fcdc
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements synchronization of jump lists between Files and Windows Explorer, replacing the existing WindowsJumpListService with a new JumpListManager that uses undocumented COM APIs (IAutomaticDestinationList and IInternalCustomDestinationList). The implementation watches for changes in both Explorer's and Files' jump list files and bidirectionally syncs them, providing a more robust jump list experience.
Changes:
- Removed the WinRT-based WindowsJumpListService and replaced it with a new JumpListManager using native COM APIs
- Added comprehensive CsWin32 definitions for undocumented COM interfaces and helper structures
- Implemented bidirectional file watcher-based synchronization between Files and Explorer jump lists
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Files.App/ViewModels/ShellViewModel.cs | Updated to call JumpListManager.AddFolderToRecentCategory instead of the removed service |
| src/Files.App/Utils/Storage/Operations/FilesystemHelpers.cs | Removed jump list service usage from filesystem operations |
| src/Files.App/Strings/en-US/Resources.resw | Removed unused "JumpListPinnedGroupHeader" resource string |
| src/Files.App/Services/Windows/WindowsJumpListService.cs | Completely removed old WinRT-based jump list service implementation |
| src/Files.App/Helpers/Application/AppLifecycleHelper.cs | Added JumpListManager initialization in STATask with environment-specific configuration |
| src/Files.App/Data/Contracts/IWindowsJumpListService.cs | Removed service interface |
| src/Files.App/App.xaml.cs | Added JumpListManager disposal on window close |
| src/Files.App.Storage/Windows/Managers/JumpListManager.cs | New comprehensive jump list manager with bidirectional sync capabilities |
| src/Files.App.Storage/Windows/Managers/JumpListItemType.cs | Removed unused enum |
| src/Files.App.Storage/Windows/Managers/JumpListItem.cs | Removed unused class |
| src/Files.App.Storage/Windows/Managers/JumpListDestinationType.cs | Removed unused enum |
| src/Files.App.Storage/Windows/Helpers/WindowsStorageHelpers.System.cs | Added system helper methods for environment variables and indirect strings |
| src/Files.App.Storage/Windows/Helpers/WindowsStorableHelpers.Storage.cs | Added GetRecentFolderPath helper |
| src/Files.App.CsWin32/NativeMethods.txt | Added numerous native methods and constants for jump list operations |
| src/Files.App.CsWin32/ManualPInvokes.cs | Reorganized and added manual P/Invoke definitions |
| src/Files.App.CsWin32/ManualMacros.cs | Added SUCCEEDED/FAILED macros for HRESULT checking |
| src/Files.App.CsWin32/ManualGuids.cs | Added GUIDs for jump list COM interfaces and classes |
| src/Files.App.CsWin32/ManualDelegates.cs | Split delegate definitions into separate file |
| src/Files.App.CsWin32/ManualConstants.cs | Split constants into separate file |
| src/Files.App.CsWin32/IInternalCustomDestinationList.cs | New undocumented COM interface definition |
| src/Files.App.CsWin32/IDCompositionTarget.cs | Split into separate file |
| src/Files.App.CsWin32/IAutomaticDestinationList.cs | New undocumented COM interface definition |
| src/Files.App.CsWin32/HeapPtr`1.cs | New heap pointer management structure |
| src/Files.App.CsWin32/HRESULT.cs | Added E_NOT_SET constant |
| src/Files.App.CsWin32/Extras.cs | Removed (split into separate files) |
| src/Files.App.CsWin32/ComPtr`1.cs | Added CopyTo and InternalAddRef methods |
| src/Files.App.CsWin32/ComHeapPtr`1.cs | Enhanced with Attach, Detach, Allocate, and Reallocate methods |
| src/Files.App.BackgroundTasks/UpdateTask.cs | Updated to delete jump list binary files on update |
| src/Files.App.BackgroundTasks/Files.App.BackgroundTasks.csproj | Added reference to Files.App.Storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
6986e15 to
954b1fc
Compare
Member
Author
|
Copilot review is so spammy :( Anyway, all done. PTAL. |
Member
|
The list of pinned items in the sidebar no longers updates when I make a change from File Explorer. |
Member
|
After the most recent commit, any changes to the jumplist causes the app to freeze. |
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
77c2c7a to
e236a0d
Compare
| if (!fRes) return null; | ||
|
|
||
| fixed (char* pSource = source) | ||
| hr = PInvoke.SHLoadIndirectString(pSource, pszBuffer.Get(), 1024U, null); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0x5bfa
force-pushed
the
5bfa/CQ-JumpListManager
branch
from
ad2854f to
2326930
Compare
Comment on lines
+56
to
+60
| public bool Allocate(nuint cch) | ||
| { | ||
| _ptr = (T*)NativeMemory.Alloc(cch); // malloc() | ||
| return _ptr is not null; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+66
to
+70
| public WindowsStorable() | ||
| { | ||
| void* globalInterfaceTable; | ||
| PInvoke.CoCreateInstance(CLSID.CLSID_StdGlobalInterfaceTable, null, CLSCTX.CLSCTX_INPROC_SERVER, IID.IID_IGlobalInterfaceTable, &globalInterfaceTable); | ||
| _globalInterfaceTable = (IGlobalInterfaceTable*)globalInterfaceTable; |
There was a problem hiding this comment.
Bug: The CoCreateInstance call in the WindowsStorable constructor does not check the returned HRESULT, which can lead to a null pointer dereference if the call fails.
Severity: HIGH
Suggested Fix
Check the HRESULT returned by PInvoke.CoCreateInstance. If it indicates failure, throw an exception to prevent the object from being created in an invalid state. This aligns with the error handling pattern used for similar COM object creations elsewhere in the repository, such as using .ThrowIfFailedOnDebug().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/Files.App.Storage/Windows/WindowsStorable.cs#L66-L70
Potential issue: The constructor for `WindowsStorable` calls `PInvoke.CoCreateInstance`
to create a `GlobalInterfaceTable` but does not check the returned HRESULT for failure.
If this call fails, for instance due to low system resources or COM initialization
issues, the `_globalInterfaceTable` field will be an invalid or null pointer. Subsequent
access to properties like `ThisPtr` or `ContextMenu` will attempt to dereference this
pointer without a null check, leading to a null pointer dereference and an application
crash. This is inconsistent with other parts of the codebase where `CoCreateInstance`
results are always checked.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved / Related Issues
TODOs
Steps used to test these changes