Skip to content

MudBaseInput: ParameterState for Value#12268

Merged
ScarletKuro merged 12 commits intobase_valuefrom
copilot/sub-pr-12267
Dec 14, 2025
Merged

MudBaseInput: ParameterState for Value#12268
ScarletKuro merged 12 commits intobase_valuefrom
copilot/sub-pr-12267

Conversation

Copy link
Contributor

Copilot AI commented Dec 13, 2025

Migrates MudBaseInput.Value to use ParameterState pattern, consistent with previous Text migration (#12259).

Changes

MudBaseInput

  • Value ParameterState: Registered _valueState with change handler OnValueParameterChangedAsync
  • TextUpdateSuppression logic: Moved from SetParametersAsync to OnValueParameterChangedAsync to properly handle value-to-text synchronization with focus/suppression checks
  • Text/Value conflict prevention: Uses ParameterView.TryGetValue() in change handlers to detect when both Text and Value are present in the same parameter update (maintains hasValue && !hasText semantics)
  • SetParametersAsync preservation: Kept the hasValue && !hasText logic in SetParametersAsync to handle cases where Value parameter is passed unchanged (needed for text formatting)
  • ForceUpdate fix: Manually invoke ValueChanged callback when value hasn't changed but force: true (ParameterState doesn't fire callbacks for unchanged values)
  • ReadValue/WriteValue overrides: Use _valueState.Value and _valueState.SetValueAsync() instead of backing field

MudColorPicker Text/Value Synchronization Refactoring

  • TextField binding: Changed from computed value @GetColorTextValue(_valueState.Value) to _textState.Value to enable Blazor parameter change detection
  • ColorPickerMode change handler: Added OnColorPickerModeChangedAsync to update Text when display mode changes (RGB → HSL → HEX)
  • Initial Text synchronization: Text is initialized when Value is first set in OnValueChangeHandlerAsync
  • Invalid input handling: When color parsing fails, _textState.SetValueAsync() is called to reset Text, which triggers TextField to receive updated parameter and reset its display
  • Root cause fix: Solves the core issue where Blazor doesn't pass unchanged parameters - by updating _textState, we force Blazor to pass the Text parameter to TextField, enabling proper reset on invalid input

Test Results

  • TextField_TextUpdateSuppression - Fixed
  • Select_ValueChangeEventCountTest - Fixed
  • SetColorInput_InvalidNoChange - Fixed
  • NumericFieldTestCultureFormat - Fixed (regression introduced and resolved)

Architecture Improvements

ParameterView-Based Conflict Detection

Eliminated the _textParameterChanged flag by using ParameterView.TryGetValue() within change handlers. Each ParameterChangedEventArgs now includes a snapshot of the ParameterView, allowing handlers to check if other related parameters are also present in the same update. This approach is thread-safe by design and eliminates the need for manual flag management or atomic operations.

Two-Way Binding Edge Case

The refactoring addresses two critical edge cases in two-way binding scenarios:

  1. ColorPicker invalid input: When TextField receives invalid input and ColorPicker's parsing fails, the TextField's Value prop doesn't change, so Blazor doesn't pass it as a parameter. By switching to _textState.Value binding and keeping Text synchronized with Value through ParameterState change handlers, TextField now properly receives parameter updates and resets its display.
  2. NumericField formatting: When Value is set internally and passed back through two-way binding with the same value, Blazor doesn't pass it as a parameter. The SetParametersAsync logic ensures Text is updated from Value even when Value parameter is present but unchanged, enabling proper formatting.

Checklist:

  • I've read the contribution guidelines
  • My code follows the style of this project
  • I've added or updated relevant unit tests

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 13, 2025 20:10
- Move Text-from-Value update logic from SetParametersAsync to OnValueParameterChangedAsync
- Add _textParameterChanged flag to prevent Text update when both Text and Value change
- Manually invoke ValueChanged callback when ForceUpdate is called with unchanged value
- Fixes TextField_TextUpdateSuppression and Select_ValueChangeEventCountTest

Co-authored-by: ScarletKuro <[email protected]>
…d values

- Add call to _textState.SetValueAsync when color parsing fails
- This attempts to reset the TextField to show the valid color
- 2 of 3 tests now passing (TextField_TextUpdateSuppression, Select_ValueChangeEventCountTest)
- SetColorInput_InvalidNoChange still fails due to ParameterState synchronization issue

Co-authored-by: ScarletKuro <[email protected]>
Copilot AI changed the title [WIP] Fix MudBaseInput to use ParameterState for Value MudBaseInput: ParameterState for Value Dec 13, 2025
Copilot AI requested a review from ScarletKuro December 13, 2025 20:27
- Add _inputResetKey counter to force TextField recreation on invalid input
- Increment key when color parsing fails in SetInputStringAsync
- Add @key directive to HEX MudTextField to bind to _inputResetKey
- This forces Blazor to recreate TextField, resetting its display to valid color
- All 3 tests now passing: TextField_TextUpdateSuppression, Select_ValueChangeEventCountTest, SetColorInput_InvalidNoChange

Co-authored-by: ScarletKuro <[email protected]>
@ScarletKuro
Copy link
Member

@copilot I added ParameterView to ParameterChangedEventArgs, not sure if that somehow helps to get rid of _textParameterChanged. examples are ParameterStateDependencyComp1 and ParameterStateDependencyComp2.
Either you can have read from ParameterChangedEventArgs, or make _valueState and _textState to have one shared change handler, but then only available argument is ParameterView, but in this case ParameterState will optimize and call only one handler and not two seperate in case value and text change.
If we can't get rid of _textParameterChanged then maybe we should make it atomic via interlock? im scared of multiple SetParametersAsync calls

Copilot AI and others added 3 commits December 14, 2025 01:35
- Remove the second reset at end of SetParametersAsync
- Flag should only be reset before processing parameters, not after
- The second reset was undoing the flag set by OnTextParameterChangedAsync
- Add StateHasChanged after UpdateTextPropertyAsync in OnValueParameterChangedAsync to ensure child components receive updated Text parameter

Co-authored-by: ScarletKuro <[email protected]>
…pdate logic

- Restore text-from-value update in SetParametersAsync for unchanged Value parameters
- ParameterState only fires OnValueParameterChangedAsync when value CHANGES
- But we need to update Text even when Value is passed unchanged (for formatting)
- Keep the hasValue && !hasText check in SetParametersAsync to handle this case
- Fixes NumericFieldTestCultureFormat test failure

Co-authored-by: ScarletKuro <[email protected]>
- Check ParameterView.TryGetValue in OnTextParameterChangedAsync to see if Value is also present
- Check ParameterView.TryGetValue in OnValueParameterChangedAsync to see if Text is also present
- Eliminates need for _textParameterChanged flag and its reset logic
- Cleaner approach using ParameterView snapshot provided by ParameterState
- Addresses concurrency concerns mentioned in code review

Co-authored-by: ScarletKuro <[email protected]>
Copy link
Contributor Author

Copilot AI commented Dec 14, 2025

@copilot I added ParameterView to ParameterChangedEventArgs, not sure if that somehow helps to get rid of _textParameterChanged. examples are ParameterStateDependencyComp1 and ParameterStateDependen...

Implemented the ParameterView approach in commit 5572c3b:

Changes:

  • Removed _textParameterChanged flag and its reset logic entirely
  • OnTextParameterChangedAsync now checks arg.ParameterView.TryGetValue<T?>(nameof(Value), out _) to see if Value is also present
  • OnValueParameterChangedAsync now checks arg.ParameterView.TryGetValue<string?>(nameof(Text), out _) to see if Text is also present
  • This approach uses the ParameterView snapshot provided by ParameterState, similar to ParameterStateDependencyComp2 example

Benefits:

  • No need for manual flag management or atomic operations
  • Thread-safe by design (ParameterView is a snapshot)
  • Cleaner, more maintainable code
  • All 4 tests passing

The NumericFieldTestCultureFormat issue was also fixed in commit eada7fd by restoring the SetParametersAsync logic for unchanged Value parameters (needed for formatting).

@ScarletKuro ScarletKuro marked this pull request as ready for review December 14, 2025 02:03
@ScarletKuro ScarletKuro merged commit 5bdf7dd into base_value Dec 14, 2025
5 checks passed
@ScarletKuro ScarletKuro deleted the copilot/sub-pr-12267 branch December 14, 2025 02:09
@danielchalmers danielchalmers added the refactor Reorganizes code and has no changes to the API or functionality in the main library label Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Reorganizes code and has no changes to the API or functionality in the main library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants