Skip to content

Comments

MudSelect: use ParameterState#12244

Merged
ScarletKuro merged 22 commits intodevfrom
copilot/rewrite-mudselect-selectedvalues
Dec 12, 2025
Merged

MudSelect: use ParameterState#12244
ScarletKuro merged 22 commits intodevfrom
copilot/rewrite-mudselect-selectedvalues

Conversation

Copy link
Contributor

Copilot AI commented Dec 11, 2025

Refactored MudSelect<T> and MudAutocomplete<T> to use the ParameterState framework for managing parameters, eliminating logic in parameter getters/setters and preventing unobserved async discards.

Changes

MudSelect ParameterState Refactoring

Core Refactoring:

  • Converted SelectedValues to auto-property with [Parameter, ParameterState]
  • Added ParameterState<IEnumerable<T?>?> backing field with event callback and change handler
  • Moved setter logic into OnSelectedValuesChangedAsync handler
  • Replaced all SelectedValuesChanged.InvokeAsync() calls with _selectedValuesState.SetValueAsync()
  • Internal _selectedValues HashSet now acts as the single source of truth for UI operations
  • Defensive Copy: OnSelectedValuesChangedAsync creates a new HashSet from incoming values to prevent shared references between parent and child components
  • Event Callback Fix: All SetValueAsync calls now create new HashSet instances to ensure ParameterState's comparer detects changes and fires events correctly
  • Constructor Initialization: Set SelectedValues = new HashSet<T?>() in constructor to ensure ParameterState never holds null
  • FieldChanged Guard: Only call FieldChanged after first render using HasRendered check to prevent firing during initialization with non-default values

Comparer Handling:

  • Converted Comparer to auto-property with [Parameter, ParameterState(ParameterUsage = None)] (read-only from parent)
  • Added OnComparerChangedAsync handler to refresh internal HashSet when comparer changes
  • Use custom Comparer for value equality checks in single-select mode instead of always using EqualityComparer<T>.Default

ParameterState Synchronization:

  • Updated SelectAndHighlightItem (used by keyboard navigation) to publish to ParameterState
  • Ensured SelectedValues is synchronized even when value doesn't change in single-select early return path
  • Fixed all code paths that update _selectedValues internal HashSet to also update _selectedValuesState

MudSelectItem ToStringFunc Fix:

  • Fixed MudSelectItem.DisplayString to properly use parent's ToStringFunc for complex types
  • Added internal ConvertValueToString method to MudSelect that wraps ConvertSet
  • Previously, complex types were showing their full type name (e.g., MudBlazor.UnitTests.TestComponents.Select.SelectCustomToStringTest+Pizza) instead of the custom string representation
  • Now properly displays custom string values (e.g., "Cardinale", "Diavolo", "Margarita") when ToStringFunc is provided

New Utility:

  • Added SequenceComparer<T> for element-wise IEnumerable comparison
  • Used by ParameterState via .WithComparer(() => new SequenceComparer<T?>(Comparer))

External Access:

  • Updated MudSelectItem<T> to use GetState(x => x.SelectedValues) for external property access
  • Added using MudBlazor.Extensions for GetState extension method

Test Updates:

  • Updated all SelectTests.cs to use GetState(x => x.SelectedValues) instead of direct property access
  • Fixed test components (MultiSelectWithCustomComparerTest.razor) to create new collection instances instead of mutating existing ones when updating bound parameters
  • Fixed SelectTestCustomToString test bug where all assertions were checking items[0] instead of items[0-3]

MudAutocomplete ParameterState Refactoring

Core Refactoring:

  • Converted Open to auto-property with [Parameter, ParameterState]
  • Added ParameterState<bool> backing field with event callback registration
  • Replaced all Open property reads with _openState.Value
  • Replaced all Open assignments with await _openState.SetValueAsync()
  • Updated MudAutocomplete.razor to use _openState.Value for bindings
  • Removed temporary _open field workarounds that prevented OpenChanged events

Test Updates:

  • Updated AutocompleteTests.cs to use GetState(x => x.Open) pattern
  • Added using MudBlazor.Extensions for GetState extension method

Pattern

MudSelect Pattern

Before (forbidden):

public IEnumerable<T?>? SelectedValues
{
    set
    {
        _selectedValues = new HashSet<T?>(value);
        SelectedValuesChanged.InvokeAsync(_selectedValues); // Unobserved async!
    }
}

After (required):

[Parameter, ParameterState]
public IEnumerable<T?>? SelectedValues { get; set; }

// Constructor initialization to prevent null
public MudSelect()
{
    SelectedValues = new HashSet<T?>();
    // ... register ParameterState
}

private async Task OnSelectedValuesChangedAsync(ParameterChangedEventArgs<IEnumerable<T?>?> arg)
{
    // Make defensive copy to avoid shared references
    _selectedValues = new HashSet<T?>(arg.Value ?? Enumerable.Empty<T?>(), Comparer);
    
    // Only fire FieldChanged after first render
    if (HasRendered)
    {
        FieldChanged(_selectedValues);
    }
    // Properly awaited operations...
}

// Publishing changes (create new instance for proper change detection):
await _selectedValuesState.SetValueAsync(new HashSet<T?>(_selectedValues, Comparer));

MudAutocomplete Pattern

Before (forbidden):

public bool Open
{
    get => _open;
    protected set
    {
        if (_open == value) return;
        _open = value;
        OpenChanged.InvokeAsync(_open).CatchAndLog(); // Unobserved async!
    }
}

After (required):

[Parameter, ParameterState]
public bool Open { get; set; }

public MudAutocomplete()
{
    // ... other initialization
    using var registerScope = CreateRegisterScope();
    _openState = registerScope.RegisterParameter<bool>(nameof(Open))
        .WithParameter(() => Open)
        .WithEventCallback(() => OpenChanged);
}

// Reading the value:
if (_openState.Value) { ... }

// Publishing changes:
await _openState.SetValueAsync(false);

Key Learnings

ParameterState Change Detection:

  • SetValueAsync only fires events if the comparer detects a change
  • Passing the same reference repeatedly causes the comparer to see no change
  • Solution: Always create new collection instances when calling SetValueAsync

Defensive Copying:

  • Parent and child must not share collection references
  • If they share a reference, parent mutations won't be detected by ParameterState's comparer
  • Solution: Create defensive copy in change handlers

Parent Component Pattern:

  • Parent components must create NEW collection instances when updating bound parameters
  • Mutating an existing collection and calling StateHasChanged() won't trigger ParameterState change detection
  • Example: _selected = new HashSet<T?> { item1, item2 }; instead of _selected.Clear(); _selected.Add(item1);

ParameterState Synchronization:

  • All code paths that update internal state must also update ParameterState when changes should be published to parent
  • Solution: Call _parameterState.SetValueAsync to keep ParameterState synchronized with parent

Initialization:

  • Set default value in constructor to prevent GetState from returning null (for collection types)
  • Use HasRendered flag to prevent side effects from firing during component initialization

ToStringFunc Display:

  • Child components must use the parent's conversion methods to properly handle custom conversion functions
  • Direct use of converters bypasses custom conversion logic
  • Solution: Add internal conversion methods that child components can access

Test Results

✅ MudSelect: All 71 SelectTests passing (100%)
🔄 MudAutocomplete: 123/131 AutocompleteTests passing (93.9%)

MudAutocomplete Failing Tests (8 tests):

  • Autocomplete_ChangeBoundValue
  • Autocomplete_OpenChanged_SelectOptionAsync
  • Autocomplete_OpenOnFocusShouldWork(True)
  • Autocomplete_Should_Close_OnTab
  • Autocomplete_Should_SelectValue_On_Tab_With_SelectValueOnTab
  • AutocompleteCoercionTest
  • OnTextChanged_CoerceValueAndNotCoerceTextAndImmediateAndDebounce_SetValueImmediatelyButDelaysMenuOpening
  • OnTextChanged_WithCoerceValueAndNotCoerceTextAndImmediateNotDebounce_SetValueAndOpenMenuImmediately

The remaining AutocompleteTests failures are under investigation and appear to be timing/async-related issues that need further debugging.

Checklist:

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

This section details on the original issue you should resolve

<issue_title>Rewrite MudSelect SelectedValues to use ParameterState</issue_title>
<issue_description>

Rewrite MudSelect (MudSelect.razor / MudSelect.razor.cs) to use ParameterState for SelectedValues

1. Convert SelectedValues into an auto-property with [ParameterState]

Replace the existing implementation with:

[Parameter, ParameterState]
[Category(CategoryTypes.FormComponent.Data)]
public IEnumerable<T?>? SelectedValues { get; set; }

Add the backing field:

private readonly ParameterState<IEnumerable<T?>?> _selectedValuesState;

In the end of the constructor, add:

_selectedValuesState = registerScope
    .RegisterParameter<IEnumerable<T?>?>(nameof(SelectedValues))
    .WithParameter(() => SelectedValues)
    .WithEventCallback(() => SelectedValuesChanged)
    .WithChangeHandler(OnSelectedValuesChangedAsync)
    .WithComparer(() => new SequenceComparer<T?>(Comparer));

2. Move old SelectedValues.set logic into a handler

Implement:

private Task OnSelectedValuesChangedAsync(ParameterChangedEventArgs<IEnumerable<T?>?> arg)
{
    // Migrate old SelectedValues setter logic here (you might need to adapt it)
}

3. Make Comparer an auto-property

Replace the old implementation with:

public IEqualityComparer<T?>? Comparer { get; set; }

If old code used Comparer = value; side effects, migrate them into:

registerScope.RegisterParameter<IEqualityComparer<T?>?>(nameof(Comparer))
    .WithParameter(() => Comparer)
    .WithChangeHandler(OnComparerChangedAsync);

private Task OnComparerChangedAsync(ParameterChangedEventArgs<IEqualityComparer<T?>?> arg)
{
    // Migrate old Comparer.set logic here
}

4. Replace usage of SelectedValues with ParameterState

Where the component previously read values:

var x = SelectedValues

replace with:

var x = _selectedValuesState.Value;

Where the code previously wrote to the parameter:

If it was:

SelectedValuesChanged.InvokeAsync(newValue)

→ replace with:

await _selectedValuesState.SetValueAsync(newValue);

If it was:

SelectedValues = newValue;

→ usually replace with:

await _selectedValuesState.SetValueAsync(newValue);

BUT do not blindly replace all assignments

In Blazor it is forbidden for a component to directly assign to a [Parameter] property to simulate an incoming parameter change — updates must go through the event callback (e.g. SelectedValuesChanged) so the parent stays authoritative. With ParameterState we use a shim that automates this pattern: never write directly to SelectedValues. Instead, use the internal HashSet<T?> for all local mutations and call _selectedValuesState.SetValueAsync(...) to publish the final value back to the parent. That SetValueAsync call is the equivalent of raising the event callback and will invoke the change pipeline you registered with WithEventCallback/WithChangeHandler.

  • If the old code assigned the parameter as the final operation (no validation/side-effects required afterward), replace it with await _selectedValuesState.SetValueAsync(finalEnumerable) (or push the internal HashSet contents).
  • If the old code assigned the parameter as part of a sequence that required validation, normalization, or other side-effects to run first, keep the new flow identical in ordering: first update the internal HashSet<T?>, then run validation/normalization/side-effects, and only when those succeed call await _selectedValuesState.SetValueAsync(_selectedInternal) to publish. If validation fails, do not call SetValueAsync (that preserves the previous behavior where the parameter wasn't updated).
  • If the previous code intentionally mutated the parameter for purely internal bookkeeping (not intended to notify the parent), replace that with internal-only mutations on the HashSet<T?> and do not call SetValueAsync. In short: SetValueAsync == "publish to parent (raise change)"; internal HashSet manipulations == "local state only".

5. Introduce an internal HashSet<T?>

Add an internal list to manage values without re-enumeration:

private HashSet<T?> _selectedInternal;

You must also introduce an internal HashSet<T?> that represents the current selected values inside the component. All add/remove/clear operations should be done on this internal set, not directly on SelectedValues or _selectedValuesState.Value. The internal set must be created using the same Comparer as the component. After the internal list is modified, you must push its contents to the parent by calling _selectedValuesState.SetValueAsync(_internalHashSet). In addition, you must keep the internal set synchronized with updates coming from the parent: when _selectedValuesState changes due to parent re-render, read `_selectedValues...


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…SelectedValues

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
Copilot AI changed the title [WIP] Rewrite MudSelect SelectedValues to use ParameterState Rewrite MudSelect to use ParameterState for SelectedValues Dec 11, 2025
Copilot AI requested a review from ScarletKuro December 11, 2025 06:23
Copilot AI and others added 6 commits December 11, 2025 06:33
Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
…arer

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
…Async

Create new HashSet instances when calling SetValueAsync to ensure ParameterState's
comparer detects value changes. Previously passing the same _selectedValues reference
caused the comparer to see no change and skip firing SelectedValuesChanged events.

Progress: 65/70 SelectTests passing (was 57/70)

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
The test modifies the bound collection directly but needs to call StateHasChanged
to trigger parent re-render so ParameterState receives the new value.

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
Changed from mutating readonly HashSet to creating new instance so ParameterState
detects the reference change. Also removed readonly modifier to allow reassignment.

Progress: 66/70 SelectTests passing (was 65/70)

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
…nces

Create new HashSet from incoming SelectedValues to prevent parent and child
from sharing the same collection reference. This ensures ParameterState properly
detects changes when parent modifies and re-passes the collection.

Progress: 67/70 SelectTests passing (was 66/70)

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
Copilot AI and others added 2 commits December 11, 2025 07:13
Only call FieldChanged in OnSelectedValuesChangedAsync after first render to avoid
triggering field change events during component initialization with non-default values.

Progress: 68/70 SelectTests passing (was 67/70)

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
Initialize SelectedValues to empty HashSet in constructor to ensure GetState never
returns null. Update SelectAndHighlightItem to publish to ParameterState. Use
Comparer for value equality check in single-select mode. Ensure SelectedValues is
always synchronized even when value doesn't change.

All 70 SelectTests now passing! ✅

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
@ScarletKuro ScarletKuro marked this pull request as ready for review December 11, 2025 07:38
Copy link
Contributor

@versile2 versile2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looked real strong. Pretty close.

@ScarletKuro
Copy link
Member

@copilot I found problem with your implementation.
I added new test SelectTestCustomToString, when you use Comlex type, and define custom ToStringFunc, then when you open the select popup, it shows MudBlazor.UnitTests.TestComponents.Select.SelectCustomToStringTest+Pizza instead of the string that is defined in the ToStringFunc.
This is an example:

<MudText Typo="Typo.h6" Class="mud-width-full">@(_pizza is null ? "Nothing selected." : $"Pizza: {_pizza.Name}")</MudText>

<MudSelect T="Pizza" @bind-Value="@_pizza" ToStringFunc="@_toStringFunc" Label="Select your pizza" Variant="Variant.Outlined" Clearable>
    <MudSelectItem Value="@(new Pizza("Cardinale"))" />
    <MudSelectItem Value="@(new Pizza("Diavolo"))" />
    <MudSelectItem Value="@(new Pizza("Margarita"))" />
    <MudSelectItem Value="@(new Pizza("Spinaci"))" />
</MudSelect>

@code {
    private Pizza _pizza = new("Diavolo");
    private readonly Func<Pizza,string> _toStringFunc = p => p?.Name;

    public class Pizza(string name)
    {
        public string Name { get; } = name;

        // Note: this is important so the select can compare pizzas
        public override bool Equals(object obj) => obj is Pizza other && string.Equals(Name, other.Name, StringComparison.Ordinal);

        // Note: this is important so the select can compare pizzas
        public override int GetHashCode() => Name.GetHashCode();
    }
}

I used the same example inside the SelectTestCustomToString.

Added internal ConvertValueToString method to MudSelect that delegates to ConvertSet,
allowing MudSelectItem to properly display values using the parent's ToStringFunc.
This fixes the issue where complex types were showing their type name instead of
the custom string representation.

Also fixed test bug where all assertions were checking items[0] instead of items[0-3].

All 71 SelectTests passing ✅

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
@ScarletKuro ScarletKuro merged commit 0f0e36f into dev Dec 12, 2025
6 of 8 checks passed
@ScarletKuro ScarletKuro deleted the copilot/rewrite-mudselect-selectedvalues branch December 12, 2025 12:54
@ScarletKuro ScarletKuro added breaking change This change will require consumer code updates refactor Reorganizes code and has no changes to the API or functionality in the main library labels Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates 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.

Rewrite MudSelect SelectedValues to use ParameterState

4 participants