ParameterState: Func Comparer, implicit operator support

[ScarletKuro]

Description

IParamterState removal

As we agreed with @henon we drop the IParamterState, the existing ParameterState was renamed to ParameterStateInternal, and new ParameterState class was created that has the same definition as IParamterState.
This was necessary because implicit/explicit operators do not support in or out interfaces.

A support to convert ParameterState -> T was added.

A new RegisterParameterBuilder builder was added.

Do not confuse with the existing ParameterAttachBuilder. They work differently under the hood so we do need to keep the ParameterAttachBuilder too. The RegisterParameterBuilder was created specifically for MudComponentBase so that you could also use the builder, instead of the overloads, the problem is that with the new Func Comparer (described below) and the static Comparer it would create 5 new overloads which is not desired and hard to maintain.

Code example:

ParameterState<double> someParameter = RegisterParameterBuilder<double>(nameof(DoubleParam))
	.WithGetParameterValueFunc(() => DoubleParam)
	.WithParameterChangedHandler(ParameterChangedHandler)
	.WithComparer(() => DoubleEqualityComparer);

You do not need to call .Attach() method since it also supports implicit operator that will do the job for you.

For now I left the RegisterParameter for backward compatibility as legacy, we might cleanup it when we decide to make this public.

Support for Func<IEqualityComparer<T>>

The initial problem was described here: #8416 (comment)
The problem is that sometimes the parameter has an associated comparer that is also marked as Blazor's [Parameter] and it could be changed at any time. A good example is the MudTreeView component. The old arhitecture allowed only a "static" comparer that do not change during ParamterState lifetime.

Notice this special case:

MudBlazor/src/MudBlazor/State/ParameterStateInternalOfT.cs

Lines 130 to 139 in a84fe3c

	// This handles a very special case when the Parameter and the associated Comparer change in razor syntax at same time.
	// Then we need to extract it manually if it exists, otherwise the HasParameterChanged will use a stale comparer.
	// The problem happens because blazor will call the parameters.SetParameterProperties(this) only after this method, this means the new comparer is not set yet and comparerFunc returns an old one.
	if (!string.IsNullOrEmpty(Metadata.ComparerParameterName))
	{
	if (parameters.TryGetValue<IEqualityComparer<T>>(Metadata.ComparerParameterName, out var newComparer))
	{
	comparer = newComparer;
	}
	}

As described in the code, we need it when the parameter and the associated comparer change at same time.
I noticed this a problem when I called in bUnit:

comp.SetParametersAndRender(parameters => parameters
	.Add(parameter => parameter.DoubleParam, 10002f)
	.Add(parameter => parameter.DoubleEqualityComparer, new DoubleEpsilonEqualityComparer(0.00001f)));

To understand it better here is the picture how it looks in the debug mode:

To support this we had to add the CallerArgumentExpression to the WithComparer and new rule ComparerParameterLambdaExclusion to extract the name from lambda.

Without this special case the SwapComparerAtSameTimeIntegrationTest and SetParametersAsync_FuncCustomComparerAsParameter_Swap tests would fail!

NB! To use this Func<IEqualityComparer<T>> you do need to use the RegisterParameterBuilder that has the associated WithComparer

How Has This Been Tested?

New NUnit + bUnit tests, current tests.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[codecov]

Codecov Report

Attention: Patch coverage is 98.21429% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 90.03%. Comparing base (28bc599) to head (2167c99).
Report is 26 commits behind head on dev.

❗ Current head 2167c99 differs from pull request most recent head 68468a1. Consider uploading reports for the commit 68468a1 to get more accurate results

Files	Patch %	Lines
...lazor/State/Builder/RegisterParameterBuilderOfT.cs	90.47%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8629      +/-   ##
==========================================
+ Coverage   89.82%   90.03%   +0.20%     
==========================================
  Files         412      418       +6     
  Lines       11878    12027     +149     
  Branches     2364     2372       +8     
==========================================
+ Hits        10670    10828     +158     
+ Misses        681      666      -15     
- Partials      527      533       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ScarletKuro]

One disadvantage is that current ComparerParameterLambdaExclusion is sensitive to spaces since it's using just replace, and if you type ()=>(no space after ()) if will not trigger. Ideas are welcome.

[ScarletKuro]

@henon Should I split the WithComparer in the RegisterParameterBuilder to WithStaticComparer and WithParameterComparer so it would be more clear for non internal mudblazor developers?

[henon]

One disadvantage is that current ComparerParameterLambdaExclusion is sensitive to spaces since it's using just replace, and if you type ()=>(no space after ()) if will not trigger. Ideas are welcome.

What about .Split("=>")[1].Trim() ? Should be very performant

[henon]

I like the builder very much. What if we completely switch to the builder and remove the old register overloads?

[henon]

With the naming I proposed it would be:

var someParameter = RegisterParameterBuilder<double>(nameof(DoubleParam))
	.WithParameterFunc(() => DoubleParam)
	.WithEventCallbackFunc(() => DoubleParamChanged)
	.WithChangeHandler(OnDoubleParamChanged)
	.WithComparer(() => DoubleEqualityComparer);

I think in this case we can even drop the Func postfix as funcs are mandatory in this case anyway so it would be even more lean:

var someParameter = RegisterParameterBuilder<double>(nameof(DoubleParam))
	.WithParameter(() => DoubleParam)
	.WithEventCallback(() => DoubleParamChanged)
	.WithChangeHandler(OnDoubleParamChanged)
	.WithComparer(() => DoubleEqualityComparer);

[ScarletKuro]

Thanks for the feedback, will do the changes later.

[henon]

I noticed it's quite easy to forget to call .Attach()

I agree. One way to deal with this would be to change to this kind of API (I know it sucks to make such changes now after having just refactored everything):

Before:

            RegisterParameterBuilder<int>(nameof(Spacing))
                .WithParameter(() => Spacing)
                .WithChangeHandler(() => _childrenNeedUpdates = true)
                .Attach();

After (no need to attach, and no need to have Builder in the Register method name):

            RegisterParameter<int>(nameof(Spacing)), builder => builder
                .WithParameter(() => Spacing)
                .WithChangeHandler(() => _childrenNeedUpdates = true)
            );

[ScarletKuro]

After (no need to attach, and no need to have Builder in the Register method name):

            RegisterParameter<int>(nameof(Spacing)), builder => builder
                .WithParameter(() => Spacing)
                .WithChangeHandler(() => _childrenNeedUpdates = true)
            );

Eh, I think I don't like this construction and I would just recommend for non MudBlazor developers to use this:

        ParameterState<double> _ = RegisterParameterBuilder<double>(nameof(DoubleParam))
            .WithParameter(() => DoubleParam)
            .WithChangeHandler(ParameterChangedHandler)
            .WithComparer(() => DoubleEqualityComparer);

to avoid any kind of problems

[henon]

By the way, why is the Attach even necessary when the ParameterState is not stored in a field? Isn't it internally hooked in a dictionary?

[ScarletKuro]

By the way, why is the Attach even necessary when the ParameterState is not stored in a field? Isn't it internally hooked in a dictionary?

MudBlazor/src/MudBlazor/State/Builder/RegisterParameterBuilderOfT.cs

Lines 176 to 191 in d16001e

	public ParameterStateInternal<T> Attach()
	{
	ArgumentNullException.ThrowIfNull(_parameterName);
	var parameterState = ParameterStateInternal<T>.Attach(
	new ParameterMetadata(_parameterName, _handlerName, _comparerParameterName),
	_getParameterValueFunc ?? throw new ArgumentNullException(nameof(_getParameterValueFunc)),
	_eventCallbackFunc,
	_parameterChangedHandler,
	_comparerFunc);
	_parameterSetRegister.Add(parameterState);
	return parameterState;
	}

Because it glues everything together and pokes IParameterSetRegister

that will trigger this:

MudBlazor/src/MudBlazor/Base/MudComponentBase.RegisterParameter.cs

Lines 7 to 10 in d16001e

	public abstract partial class MudComponentBase : IParameterSetRegister
	{
	/// <inheritdoc />
	void IParameterSetRegister.Add<T>(ParameterStateInternal<T> parameterState) => Parameters.Add(parameterState);

to add the state to the Parameters.

When you are using field it just getting triggered by the implicit cast automatically:

MudBlazor/src/MudBlazor/State/Builder/RegisterParameterBuilderOfT.cs

Line 197 in d16001e

public static implicit operator ParameterState<T>(RegisterParameterBuilder<T> builder) => builder.Attach();

[henon]

I see. Couldn't the ComponentBase keep track of all registered ParameterStateInternal and call Attach on all unattached i.e. in OnInitialized ?

[ScarletKuro]

I see. Couldn't the ComponentBase keep track of all registered ParameterStateInternal and call Attach on all unattached i.e. in OnInitialized ?

That's the thing, if Attach() is not called, it's not going to end-up in the ParameterSet aka Parameter and the ComponentBase simply doesn't know that this ParameterStateInternal exist, it can't keep track of it. Unless you create a second list where you will keep track of the builders and add them on the create method, and then anything that wasnt attached would be called, but idk is it worth it?

[henon]

I guess it depends on the point of view whether or not it's worth it. You don't have to document this case, you don't have to remember not to forget to call Attach and you don't have to debug and find the bug when you forgot to call Attach. Finally, we don't have to answer any issues of people who forgot to call Attach.

Another possibility would be to throw an exception with a good message, if unattached builders are collected by the GC.

[henon]

This Attach problem arises due to the splitting of the ParameterState into the public variant and the internal variant. If you wouldn't split them and just have one ParameterState with internal API for the builder to configure it you wouldn't even need to attach because you have a parameter state object with a parameter name even before the builder starts adding the rest of the data and you could immediately store it in the parameter set. So this would be a third option. You'd have to remove WithName, I think we don't need that anyway.

[henon]

Hehe, I like the name SmartAttachable

[ScarletKuro]

Pushed code that shows how the track can be keeped. If you are happy with the design, I can add xml documents.

This Attach problem arises due to the splitting of the ParameterState into the public variant and the internal variant. If you wouldn't split them and just have one ParameterState with internal API for the builder to configure it you wouldn't even need to attach because you have a parameter state object with a parameter name even before the builder starts adding the rest of the data and you could immediately store it in the parameter set. So this would be a third option. You'd have to remove WithName, I think we don't need that anyway.

Can you explain this better, preferably with some pseudocode.
I don't understand how the split ParameterState and ParameterStateInternal plays any role here.
What you are talking about would be possible, if ParameterState had a reference on the MudComponentBase which is possible with current design as well, I don't really wanna do, since direct unit testing(tho you could use interface) but I don't feel like ParameterState should turn in some kind of a god object doing everything by a little bit.

[henon]

Never mind, the autoattach you just implemented solves the problem just nicely.

[henon]

Pushed code that shows how the track can be keeped. If you are happy with the design, I can add xml documents.

Yes, then we should merge this ASAP before it gets unnecessarily conflicted.

[henon]

Excellent job @ScarletKuro, I can't wait to leverage the implicit cast !

[ScarletKuro]

For future I still plan to rethink all this registration.
Ideally I want something like this

using(var register = RegisterParameters())
{
     variable1 = ... register.WithName(...) etc
     variable2 = ... register.WithName(...) etc
}

With using I want that when the scope is ended it would only then register everything to ParameterSet, this would allow to use FrozenDictionary which is 2x faster on read, would be great for GetState. Because currently, event thought we are registering everything in constructor, but we don't know when the registration starts and ends, making it impossible to use FrozenDictionary. Also this should make that queue redundant.

[henon]

Another idea to consider: Add a virtual InitState() method to MudComponentBase and move all registration into InitState(). Then you can first execute the InitState and after that attach and freeze.

public class MudComponentBase : ComponentBase {
  public MudComponentBase() {
    InitParameters();    
  }

 private InitParameters() {
  InitState();
  FreezeState();
 } 
  
  protected virtual InitState() { /*to be overridden*/ }
}