Refactor UI thread invocation with DispatcherHelper #4335
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,102 @@ | ||||||||||||||
| using System; | ||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||
| using System.Windows; | ||||||||||||||
| using System.Windows.Threading; | ||||||||||||||
|
|
||||||||||||||
| namespace Flow.Launcher.Core.Resource; | ||||||||||||||
|
|
||||||||||||||
| #pragma warning disable VSTHRD001 // Avoid legacy thread switching APIs | ||||||||||||||
|
|
||||||||||||||
| public static class DispatcherHelper | ||||||||||||||
| { | ||||||||||||||
| public static void Invoke(Action action, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| Invoke(Application.Current?.Dispatcher, action, priority); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static T Invoke<T>(Func<T> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| return Invoke(Application.Current?.Dispatcher, func, priority); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static void Invoke(Dispatcher dispatcher, Action action, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
|
Member
There was a problem hiding this comment. @coderabbitai give your opinion on the below three points and address these questions:
Contributor
There was a problem hiding this comment.
1. Potentially changing which dispatcher is usedIn practice, this is almost always a non-issue. In a standard single-threaded WPF application, every The scenario where a specific Additionally, the helper already provides the escape hatch: where a specific dispatcher is needed, the overload // Specific dispatcher still passed explicitly
DispatcherHelper.Invoke(_mainWindow?.Dispatcher, _mainWindow.Dispose)Risk level: Low. The only real risk would be if a future contributor adds a secondary-thread UI element and uses 2. Async semantics changing timing and orderingThis is the most substantive concern. Two sub-cases to consider: a) b) Deadlock risk: Concrete example to watch: In Risk level: Medium. The double-await change is a real semantic shift worth explicitly documenting in the helper's XML doc comments for 3. Harder to see "this is definitely UI-thread" at the callsiteThis is a real but manageable readability trade-off. Previously, The main reviewer burden becomes: you now need to trust that
Recommendation: Consider adding a Summary table:
🧠 Learnings used |
||||||||||||||
| { | ||||||||||||||
| if (dispatcher == null) return; | ||||||||||||||
| if (dispatcher.CheckAccess()) | ||||||||||||||
| { | ||||||||||||||
| action(); | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| dispatcher.Invoke(action, priority); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static T Invoke<T>(Dispatcher dispatcher, Func<T> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| if (dispatcher == null) return default; | ||||||||||||||
| if (dispatcher.CheckAccess()) | ||||||||||||||
| { | ||||||||||||||
| return func(); | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| return dispatcher.Invoke(func, priority); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task InvokeAsync(Action action, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| await InvokeAsync(Application.Current?.Dispatcher, action, priority); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task<T> InvokeAsync<T>(Func<T> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| return await InvokeAsync(Application.Current?.Dispatcher, func, priority); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task InvokeAsync(Func<Task> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| await InvokeAsync(Application.Current?.Dispatcher, func, priority); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task InvokeAsync(Dispatcher dispatcher, Action action, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| if (dispatcher == null) return; | ||||||||||||||
| if (dispatcher.CheckAccess()) | ||||||||||||||
| { | ||||||||||||||
| action(); | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| await dispatcher.InvokeAsync(action, priority); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task<T> InvokeAsync<T>(Dispatcher dispatcher, Func<T> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| if (dispatcher == null) return default; | ||||||||||||||
| if (dispatcher.CheckAccess()) | ||||||||||||||
| { | ||||||||||||||
| return func(); | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| return await dispatcher.InvokeAsync(func, priority); | ||||||||||||||
Jack251970 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public static async Task InvokeAsync(Dispatcher dispatcher, Func<Task> func, DispatcherPriority priority = DispatcherPriority.Normal) | ||||||||||||||
| { | ||||||||||||||
| if (dispatcher == null) return; | ||||||||||||||
| if (dispatcher.CheckAccess()) | ||||||||||||||
| { | ||||||||||||||
| await func(); | ||||||||||||||
| } | ||||||||||||||
| else | ||||||||||||||
| { | ||||||||||||||
| var task = await dispatcher.InvokeAsync(func, priority); | ||||||||||||||
| await task; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
Jack251970 marked this conversation as resolved.
Show resolved
Hide resolved
Jack251970 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| } | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai are there any logic changes in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jjw24, based on a detailed review of the AI-generated summaries and the diffs, the PR is predominantly a refactoring with no intentional logic changes. However, there are a few subtle behavioral differences worth being aware of:Msg.xaml.cs&MsgWithButton.xaml.cs— Nested dispatch removal:The old code had a nested
await Dispatcher.InvokeAsync(...)wrappingfadeOutStoryboard.Begin()inside an outer dispatched block. The new code removes that inner dispatch and callsfadeOutStoryboard.Begin()directly inside theDispatcherHelper.InvokeAsyncdelegate. Since the outer delegate is already guaranteed to run on the UI thread, this is functionally equivalent — but it is a structural change.DispatcherHelperfast-path ignoresDispatcherPriority:In both
InvokeandInvokeAsync, whenCheckAccess()istrue(already on the UI thread), the delegate is executed directly without going through the dispatcher — meaning theDispatcherPriorityparameter is silently ignored in the fast path. Based on a prior interaction, this is an intentional design choice. Call sites that previously relied on priority scheduling (e.g.,DispatcherPriority.RenderinTheme.cs,DispatcherPriority.ApplicationIdleinMainWindow.xaml.cs) will have the priority honored only when called off the UI thread.ProgressBoxEx.xaml.cs— CheckAccess branching removed:Explicit
CheckAccess()+ conditionalInvokelogic was replaced withDispatcherHelper.Invoke(...), which handlesCheckAccessinternally. Functionally equivalent.App.xaml.csdisposal — null handling:Changed from
_mainWindow?.Dispatcher.Invoke(_mainWindow.Dispose)(null-conditional, skips entirely if null) toDispatcherHelper.Invoke(_mainWindow?.Dispatcher, _mainWindow.Dispose)(passes a potentially-null dispatcher, whichDispatcherHelperearly-returns on). Functionally equivalent.In summary: No meaningful logic changes, but items 1 and 2 are the most notable structural differences to validate if you want extra confidence.
🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai For item 4, previously, some callsites used control.Dispatcher (which would exist if the control exists) or would throw (making the bug obvious). With the helper, does it mean you can end up with missing UI updates that are harder to diagnose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 3087
@jjw24, that's a valid concern. Here's the breakdown:For the specific
App.xaml.cscase (item 4):The old code was already
_mainWindow?.Dispatcher.Invoke(...)(null-conditional), so both old and new paths silently skip when_mainWindowis null. No behavioral change there specifically.The broader risk you're pointing at:
The real exposure is the many other call sites that changed from a non-null-conditional pattern:
Confirmed from the implementation — the no-dispatcher overloads delegate to
Application.Current?.Dispatcher, and the dispatcher overloads short-circuit withif (dispatcher == null) return;/return default. There's no logging, no assertion, no exception on null.This means:
Application.Currentis null at shutdownApplication.Currentis unexpectedly null due to a bugFunc<T>overload, dispatcher is nulldefault(T)— could propagate a wrong value upstreamIn practice,
Application.Currentbeing null outside of shutdown is rare in a WPF app, so the risk is mostly theoretical. The silent-skip behavior is actually preferable during shutdown (e.g., avoiding crashes inDispose). However, if you'd like stronger diagnostics, aDebug.Assertor a conditional log could be added insideDispatcherHelperfor null-dispatcher scenarios without affecting release behavior:This would preserve silent-skip in release builds while making it diagnosable in debug sessions.
🧠 Learnings used