Code Quality: Replaced the StartSTA helpers with STATask.Run #17405
Conversation
0x5bfa
force-pushed
the
5bfa/CQ-StartSTA
branch
from
85b38eb to
59d2ed8
Compare
0x5bfa
force-pushed
the
5bfa/CQ-StartSTA
branch
from
c01a726 to
60c5dfc
Compare
yaira2
force-pushed
the
main
branch
2 times, most recently
from
97999e5 to
806f922
Compare
|
@0x5bfa what's the status of this PR? |
yaira2
added
the
needs - additional info
|
Ready for review and merge. |
|
Can you please resolve the merge conflicts and fill out the PR template? Thank you. |
yaira2
removed
the
needs - additional info
0x5bfa
force-pushed
the
5bfa/CQ-StartSTA
branch
2 times, most recently
from
ecf7f65 to
7e950b5
Compare
yaira2
force-pushed
the
5bfa/CQ-StartSTA
branch
from
7e950b5 to
fc6ef4b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase by replacing custom Win32Helper.StartSTATask helper methods with the new STATask.Run API. The change standardizes STA (Single-Threaded Apartment) task execution throughout the application and removes the deprecated helper methods.
Key Changes:
- Replaced all
Win32Helper.StartSTATaskcalls withSTATask.Runacross the codebase - Removed the four
StartSTATaskmethod overloads fromWin32Helper.Storage.cs - Updated
STATask.Runmethods to require a non-nullableILoggerparameter and removed redundant exception propagation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| QuickAccessWidgetViewModel.cs | Added logger parameter to STATask.Run calls for pin/unpin operations |
| FileOperationsHelpers.cs | Migrated 10 Win32Helper.StartSTATask calls to STATask.Run with logger |
| FileThumbnailHelper.cs | Updated icon loading methods to use STATask.Run |
| LaunchHelper.cs | Replaced StartSTATask calls in application launch handlers |
| LibraryManager.cs | Migrated library management operations to STATask.Run |
| WindowsQuickAccessService.cs | Updated quick access unpin operations to use STATask.Run |
| StorageTrashBinService.cs | Replaced StartSTATask in trash restoration logic |
| StorageNetworkService.cs | Migrated network operations to STATask.Run |
| Win32Helper.Storage.cs | Removed four deprecated StartSTATask method overloads |
| Win32Helper.Shell.cs | Updated shell enumeration to use STATask.Run |
| STATask.cs | Made logger parameter required and removed duplicate exception handling |
| WindowsStorableHelpers.Icon.cs | Added null logger parameter to STATask.Run call |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| HRESULT hr = storable.TryGetThumbnail(size, options, out var thumbnailData).ThrowIfFailedOnDebug(); | ||
| return thumbnailData; | ||
| }); | ||
| }, null); |
There was a problem hiding this comment.
Passing null for the logger parameter is inconsistent with other calls in this PR that use App.Logger. This means exceptions in thumbnail operations won't be logged, making debugging harder. Consider passing a logger instance here for consistency.
| }, null); | |
| }, App.Logger); |
| /// <param name="action">The work to execute in the STA thread.</param> | ||
| /// <param name="logger">A logger to capture any exception that occurs during execution.</param> | ||
| /// <returns>A <see cref="Task"/> that represents the work scheduled to execute in the STA thread.</returns> | ||
| public static Task Run(Action action, ILogger? logger) |
There was a problem hiding this comment.
The logger parameter is marked as nullable (ILogger?) but is now required based on all call sites passing App.Logger. This creates ambiguity in the API contract. Consider making it non-nullable if it's intended to be required, or provide a default parameter value if null is acceptable.
2 participants
Re: #17377
Resolved / Related Issues
Steps used to test these changes