-
Notifications
You must be signed in to change notification settings - Fork 59
Fix ListState.AddAsync callback firing with null value #2947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@copilot can you retry |
When AddAsync updates list data without updating selection, ListFeedSelection internally clears the selection to avoid out-of-range issues. This clear was syncing to the selection state, causing ForEach callbacks to fire with null. The fix adds a sentinel marker (InternalSelectionClear) to distinguish between: - Internal selection clearing due to data changes (should not sync to state) - Explicit selection changes from user/code (should sync to state) This prevents the intermediate null callback while maintaining proper selection clearing behavior when data changes. Co-authored-by: kazo0 <[email protected]>
|
|
Done! Implemented the fix in commit a1076da. The issue was that when The fix adds an Note: Unable to verify with local build due to network issues with package feeds, but CI will validate the changes. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
dr1rrb
left a comment
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.
Fix is good 👍, please add tests for this
|
@copilot can you please add runtime tests for this change? |
Added two tests to verify fix for issue #2942: - When_AddAsyncThenTrySelectAsync_Then_SelectionStateDoesNotReceiveIntermediateNone - When_AddAsyncThenTrySelectAsync_WithInitialData_Then_SelectionStateDoesNotReceiveIntermediateNone These tests verify that when AddAsync is followed by TrySelectAsync, the selection state only receives the selected value and not an intermediate None/null value. Both tests pass with the fix in place. Co-authored-by: kazo0 <[email protected]>
Runtime tests added in commit 3b7ab08. The tests verify that AddAsync followed by TrySelectAsync only triggers the ForEach callback once with the selected value, not twice (once with null, once with the value).
Added tests in commit 3b7ab08. Two test cases verify the fix:
Both tests pass, confirming the selection state only receives the selected value without intermediate None/null. |
|
@dr1rrb the tests look good to me but I'll wait for your review |
|
@kazo0 seems like this PR is ready to be merged? 😃 I am hopefull to finally be able to publish my Video Tutorials using this 😍
|
GitHub Issue (If applicable): closes #2942
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When calling
ListState.AddAsync()followed byTrySelectAsync(), theSelection/State.Empty.ForEachcallback fires twice:nullvalue (unexpected)This occurs because when
AddAsyncupdates the list data without updating the selection,ListFeedSelection.UpdateMessageAsyncinternally clears the selection to avoid out-of-range issues. This internal clearing was being synced to the selection state, causing the ForEach callback to receive an intermediateNone/nullvalue before the actual selection is set byTrySelectAsync.What is the new behavior?
The ForEach callback now only fires once with the actual selected value. Internal selection clearing due to data changes no longer syncs to the selection state, preventing the intermediate null callback.
Implementation details:
InternalSelectionClearinListFeedSelection.csto distinguish between internal selection clearing (due to data changes) and explicit selection changesUpdateMessageAsyncto mark internal clears with this sentinel markerInternalSelectionClearfrom syncing to the selection stateGiven_ListFeedSelection.csto verify the fix:When_AddAsyncThenTrySelectAsync_Then_SelectionStateDoesNotReceiveIntermediateNoneWhen_AddAsyncThenTrySelectAsync_WithInitialData_Then_SelectionStateDoesNotReceiveIntermediateNoneThis maintains proper selection clearing behavior when data changes while preventing unwanted null callbacks.
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Runresults.Other information
The fix follows existing patterns in the codebase (similar sentinel marker patterns exist in
AsyncCommand.cs). Both runtime tests pass successfully, verifying that the selection state only receives the selected value without intermediate None/null callbacks. CI will validate the changes with full build and test suite.Internal Issue (If applicable): N/A
Original prompt
This section details on the original issue you should resolve
<issue_title>[MVUX] ListState.AddAsync causes Selection/State.Empty.ForEach callback to fire with null before the actual selected value</issue_title>
<issue_description><!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!
If the matter is security related, please disclose it privately via https://github.com/nventive/Uno/security/
-->
Current behavior
When calling
.AddAsync(uri,ct), where Uri has the value ofhttps://platform.uno/studio/at this time, on aListState<Uri>.Async( ... .Selection(AnyUriState)theAnyUriState.Empty(this).ForEach(Operator callback handler should not get called withnullAND THEN with the expected Value ofhttps://platform.uno/studio/seems a bit like the
.Empty(this)might be resetting it? but should not be the case...Expected behavior
The ForEach Operator Callback handler should only get called with the value that has been added + selected!
How to reproduce it (as minimally and precisely as possible)
Repro: UnoWebViewReproApp1.zip
HistorySelectionChangedCallback Handler 1. Line (logger)CurrentUrlChangedin theAddAsyncandTrySelectAsyncLines relativly on the bottom area of the methodPlatformin the Head Navigation (most likly you will need to use the Hamburger Icon before, since we are not able to Have the WebView2 Resized automatically on Window resizing) so you will get Navigated to this pageStudioto get navigated to this.closely inspecting the logger lines producement order:
AddAsyncthe value is not null then the valid url.HistorySelectionChangedgets called withnullargument and loggs its initial lineCurrentUrlChanged > TrySelectAsyncHistorySelectionChanged >if statementwith null value, so does not enter the statement and leaves the methodTrySelectAsyncand getting send to the `HistorySel...💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.