feat!(threading): Use ValueTask for potentially synchronous operations#1693
feat!(threading): Use ValueTask for potentially synchronous operations#1693pomianowski wants to merge 10 commits intomainfrom
ValueTask for potentially synchronous operations#1693Conversation
ValueTask for async operationsValueTask for potentially synchronous operations
There was a problem hiding this comment.
Pull request overview
Updates the Wpf.Ui library’s navigation/threading async surface area to use ValueTask where operations may complete synchronously, and adjusts repo tooling/metadata to align with the next major version and new solution format.
Changes:
- Breaking change: migrate
INavigationAwaretoValueTaskand update navigation notification flow to avoid allocations for synchronous completions. - Refactor snackbar show/hide pipeline to consolidate hide logic and reduce async state-machine overhead.
- Repository/tooling updates: introduce
Wpf.Ui.slnx, adjust build/CI scripts, and bump package/assembly versioning toward 5.0.0.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Wpf.Ui/Wpf.Ui.csproj | Adds conditional package references related to ValueTask support for older TFMs. |
| src/Wpf.Ui/Extensions/ContentDialogServiceExtensions.cs | Minor object creation syntax adjustment. |
| src/Wpf.Ui/Controls/Snackbar/SnackbarPresenter.cs | Refactors snackbar lifecycle methods and cancellation behavior. |
| src/Wpf.Ui/Controls/Snackbar/Snackbar.cs | Adjusts async API flow for immediate vs queued display. |
| src/Wpf.Ui/Controls/NavigationView/NavigationViewContentPresenter.cs | Converts navigation notifications to ValueTask and adds fire-and-forget observation helper. |
| src/Wpf.Ui.Gallery/Views/Windows/MonacoWindow.xaml | Updates status bar text to reference the new solution format. |
| src/Wpf.Ui.Gallery/ViewModels/ViewModel.cs | Updates navigation hooks to ValueTask. |
| src/Wpf.Ui.Gallery/Controllers/MonacoController.cs | Refactors controller to primary constructor and removes unnecessary async/await usage. |
| src/Wpf.Ui.Extension.Template.Compact/Wpf.Ui.Compact.csproj | Updates template package references for the next major version. |
| src/Wpf.Ui.Extension.Template.Compact/ViewModels/Pages/DataViewModel.cs | Updates template navigation hooks toward ValueTask. |
| src/Wpf.Ui.Abstractions/Wpf.Ui.Abstractions.csproj | Adds conditional package reference for ValueTask support on older TFMs. |
| src/Wpf.Ui.Abstractions/Controls/NavigationAware.cs | Updates base navigation-aware class to ValueTask. |
| src/Wpf.Ui.Abstractions/Controls/INavigationAware.cs | Breaking change: interface methods now return ValueTask. |
| samples/Wpf.Ui.Demo.Mvvm/ViewModels/ViewModel.cs | Updates sample navigation hooks to ValueTask. |
| samples/Wpf.Ui.Demo.Dialogs/MainWindow.xaml.cs | Removes unnecessary async state-machine where possible for dialog show. |
| docs/index.md | Trims whitespace in docs content. |
| build.ps1 | Updates restore command to reference the new solution format. |
| Wpf.Ui.slnx | Adds new XML-based solution file. |
| Wpf.Ui.sln.DotSettings | Adds solution-level IDE dictionary settings. |
| Wpf.Ui.sln | Removes the legacy .sln. |
| Wpf.Ui.Library.slnf | Removes legacy solution filter. |
| Wpf.Ui.Gallery.slnf | Removes legacy solution filter. |
| Directory.Packages.props | Adds package version entry for System.Threading.Tasks.Extensions and related adjustments. |
| Directory.Build.props | Bumps repo/package version toward 5.0.0-preview.1. |
| .github/workflows/wpf-ui-cd-extension.yaml | Updates restore/build steps and formatting. |
| .github/copilot-instructions.md | Removes references to deleted solution filters. |
| .editorconfig | Updates file-glob formatting rules for solution/config files. |
| .claude/settings.local.json | Updates local tool allowlist entries. |
| // NOTE: Post hide token, can we handle it better? | ||
| await Task.Delay(300, cancellationToken); |
There was a problem hiding this comment.
HideSnackbar passes cancellationToken into the post-hide delay (Task.Delay(300, cancellationToken)) but does not handle OperationCanceledException there. If the token is canceled during this delay (e.g., when HideCurrent() cancels CancellationTokenSource), the exception will propagate and can break ShowQueuedSnackbarsAsync() / ImmediatelyDisplay() which await this task. Either don't cancel the post-hide delay, or catch cancellation there to preserve the previous "best-effort" behavior.
| // NOTE: Post hide token, can we handle it better? | |
| await Task.Delay(300, cancellationToken); | |
| try | |
| { | |
| await Task.Delay(300, cancellationToken); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| } |
| if (Content is IDisposable disposableContent) | ||
| { | ||
| disposableContent.Dispose(); | ||
| } | ||
|
|
||
| Content = null; |
There was a problem hiding this comment.
HideSnackbar disposes Content (and then sets Content = null) rather than operating on snackbarToHide. This is fragile if Content changes before the method reaches this point (e.g., future refactors that allow overlapping show/hide operations), and it can dispose/clear the wrong snackbar. Prefer disposing/clearing based on snackbarToHide and only clearing Content if it still refers to that same instance.
| if (Content is IDisposable disposableContent) | |
| { | |
| disposableContent.Dispose(); | |
| } | |
| Content = null; | |
| if (snackbarToHide is IDisposable disposableSnackbar) | |
| { | |
| disposableSnackbar.Dispose(); | |
| } | |
| if (ReferenceEquals(Content, snackbarToHide)) | |
| { | |
| Content = null; | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| ) | ||
| { | ||
| // SynchronizationContext is preserved intentionally — callers may update UI state. | ||
| await first; |
There was a problem hiding this comment.
Let's make this explicit with ConfigureAwait(true).
| { | ||
| await pending.ConfigureAwait(false); | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
| catch (Exception) | |
| catch |
| return Presenter.ImmediatelyDisplay(this); | ||
| } | ||
|
|
||
| Presenter.AddToQue(this); |
There was a problem hiding this comment.
Can we rename this to AddToQueue?
| } | ||
|
|
||
| await Task.Delay(300); | ||
| snackbarToHide.SetCurrentValue(Snackbar.IsShownProperty, false); |
There was a problem hiding this comment.
General comment here; we need to make sure we run these things on UI thread / through Dispatcher.
Pull request type
Please check the type of change your PR introduces: