Commented out the TreeView_OtherTest() for now, as it's using BL0005

Removed protected bool IsChecked, replaced with the _selectedState.Value and SetSelectedAsync.
Removed ArrowExpanded, replaced by the Expanded State.

Codecov Report

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

Project coverage is 90.10%. Comparing base (28bc599) to head (2e181a7).
Report is 65 commits behind head on dev.

@@            Coverage Diff             @@
##              dev    #8661      +/-   ##
==========================================
+ Coverage   89.82%   90.10%   +0.27%     
==========================================
  Files         412      417       +5     
  Lines       11878    12127     +249     
  Branches     2364     2388      +24     
==========================================
+ Hits        10670    10927     +257     
+ Misses        681      663      -18     
- Partials      527      537      +10     

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

@ScarletKuro I know it's kind of a little unrelated here, but while you are working on MudTreeView, I do not understand why pull request #4556 was closed, but the Items Type is still Hashset instead of IReadOnlyCollection like suggested by @henon.

Is this off topic for V7?

if we do same for Activated:

+private readonly ParameterState<bool> _activatedState;

+_activatedState = RegisterParameterBuilder<bool>(nameof(Activated))
+	.WithParameter(() => Activated)
+	.WithEventCallback(() => ActivatedChanged)
+	.WithChangeHandler(OnActivatedParameterChangedAsync);

-public bool Activated
-{
-	get => _isSelected;
-	set
-	{
-		if (_isSelected.Equals(value)) return;
-
-		_isSelected = value;
-	}
-}
+public bool Activated { get; set; }

protected override async Task OnAfterRenderAsync(bool firstRender)
{
-	if (firstRender && _isSelected)
+	if (firstRender && _activatedState.Value)

-public override async Task SetParametersAsync(ParameterView parameters)

+private Task OnActivatedParameterChangedAsync(ParameterChangedEventArgs<bool> arg)
+{
+	if (MudTreeRoot is null)
+	{
+		if (MudTreeRoot is not null)
+		{
+			await MudTreeRoot.Select(this, previousActivatedValue);
+		}
+		return Task.CompletedTask;
+	}
+
+	return MudTreeRoot.Select(this, arg.Value);
+}

-if (item.Activated)
+item.GetState<bool>(nameof(MudTreeViewItem<T>.Activated))

other changes replacing _isSelected on _activatedState.Value

Then at first glance it seems to work, but when playing with MultiSelection="false"

You will notice than sometimes you need to double click for the node to be activated, and the test
TreeView_WillUnselectItems_WhenNotMultiSelect shows that bug as it fails. if you add second

comp.FindAll("div.mud-treeview-item-content")[4].Click();

then it will pass.

It was really late for me, and I haven't found the reason yet.

@ScarletKuro I know it's kind of a little unrelated here, but while you are working on MudTreeView, I do not understand why pull request #4556 was closed, but the Items Type is still Hashset instead of IReadOnlyCollection like suggested by @henon.

Is this off topic for V7?

No, we'll change to IReadOnlyCollection just like in MudChipSet which I just reworked the same way.

Hi @belucha,
Thanks for your interest

I do not understand why pull request #4556 was closed

This PR was created in 2022 when breaking changes were not acceptable, therefore it was closed. Also, this PR is now not against the latest code base.
Now we are working on next version where breaking changes are acceptable.

but the Items Type is still Hashset instead of IReadOnlyCollection like suggested by @henon.

Because we only started to work on new version, obviously we can't change everything at once. We address things by the matter of the priority. The IReadOnlyCollection are still to come.

@henon great!
If you need help. I can support.

Added another example and found a bug

New example

I will re double check everything at night

Btw, remember talking about state comparers and collections? I am about to push a CollectionComparer that just does that: comparing readonly collections.

Keep it mind, that if you will hot swap the Comparer and the SelecteValueS it will not work as expected as the special case I mentioned before will not work with this () => new CollectionComparer<T>(Comparer)

We would need to add something like this

public RegisterParameterBuilder<T> WithComparer<TFrom>(Func<IEqualityComparer<TFrom>> comparerFromFunc, Func<IEqualityComparer<TFrom>, IEqualityComparer<T>> comparerToFunc, [CallerArgumentExpression(nameof(comparerFromFunc))] string? comparerParameterName = null)
{
	_comparerFunc = () => comparerToFunc(comparerFromFunc());
	_comparerParameterName = comparerParameterName;

	return this;
}

.WithComparer<T?>(() => Comparer, x=> new CollectionComparer<T>(x));

But I would also need to store somewhere the TFrom info and the transform lambda somewhere in the InternalParameterState

This is kind of troublesome for now.

upd: another solution would be just to use two compares for single and multiple selection, since wrapping another IEqualityComparer with another IEqualityComparer is kind of weird.

Keep it mind, that if you will hot swap the Comparer and the SelecteValueS it will not work as expected as the special case I mentioned before will not work with this () => new CollectionComparer<T>(Comparer)

Why not? Sorry I don't understand and I didn't understand the special case you are talking about. Don't you call the func for every comparison? The only concern is that a new collection comparer is created on every comparison but it would always be up to date with the latest Comparer.

Why not? Sorry I don't understand and I didn't understand the special case you are talking about.

I described it here #8629 in the Support for Func<IEqualityComparer> and there is even SS and bUnit tests that
a) proves that the problem exist
b) proves that my code solves this problem partially (just not your case with SelectedValues + Comparer, only the SeclectedValue + Comparer)

Don't you call the func for every comparison?

Yes, that's what func does. However, when the HasParamterChanged fires, it fires before the base.SetParametersAsync this means that Blazor did not update the actual properties yet (to be specific the Comparer and others), the change is only reflected in the ParameterView. They only will be when base.SetParametersAsync is called. The key problem is that the HasParamterChanged decides whatever the handler should fire or not, so you might end up in situation when the handler didn't fire when it should and vice versa. Ofc the first one is the worst scenario, because when the handler fires it does already have the updated Comparer and all other methods like SetValueAsync etc does have the updated one.

oh, so instead of calling the func you try to find the comparer in the parameterview to get the latest value?
In that case I think I'll just use the collection comparer manually in the handler event. In that case, however, it might be best if it were possible to disable the equality check mechanism for the state object. After all, I will abort anyway if I find out that the new collection and the existing collection are equal.

... or if you don't find the comparer in the view just call the func. This whole thing is a very carefully crafted academic problem that will never ever happen in reality. nobody switches comparers in the middle of the life-time of the component and we could even forbid it to make sure. Then the problem is limited only to initialization where it can be handled by simply consolidating the initial parameter values with the Comparer in OnAfterRender.

it might be best if it were possible to disable the equality check mechanism for the state object

I think this would be the best and easiest to implement.

or if you don't find the comparer in the view just call the func

It's already doing so.

This whole thing is a very carefully crafted academic problem that will never ever happen in reality

Yes, this is what I wanted to say, this is a very niche problem that is very unlikely to happen, so you shouldn't worry about it, and this is why I don't want to make it more complicated than it should be. But I just wanted to let you know, so there not just me who knows the nuances :) It's better when multiple people carries the knowledge for future.

@henon awesome work, thanks

This is kind of troublesome for now.

I don't want to make it more complicated than it should be

After sleeping over with this, I found relatively easy solution how the implementation can look like without overhauling directly the ParameterStateInternal.
I came up with an easy hot swappable wrapper for IEqualityComparer instead of the current Func<EventCallback<T>>, well it does implement Func<EventCallback<T> but it's a little more smarter than that, and I can add more generics to it without adding this information to ParameterStateInternal as it's using an interface IParameterEqualityComparerSwappable<T> : IEqualityComparer<T>.
In short it adds an new overload that looks like this

public RegisterParameterBuilder<T> WithComparer<TFrom>(Func<IEqualityComparer<TFrom>> comparerFromFunc, Func<IEqualityComparer<TFrom>, IEqualityComparer<T>> comparerToFunc, [CallerArgumentExpression(nameof(comparerFromFunc))] string? comparerParameterName = null)
{
	_comparer = new ParameterEqualityComparerTransformSwappable<TFrom, T>(comparerFromFunc, comparerToFunc);
	_comparerParameterName = comparerParameterName;

	return this;
}

And instead of this usage:

.WithComparer(x => new CollectionComparer<T>(Comparer));

It's:

.WithComparer(() => Comparer, x => new CollectionComparer<T>(x));

When you don't need any transformation the old syntax stays

.WithComparer(() => Comparer);

And the unit tests shows that it does solve the problem and doesn't break the existing logic.
I will later make an PR after some cleanup etc.

I just thought it would be "nice" to have, because I saw that MudChipSet also requires this.

Just wanted to chime in here with a few questions:

• With new selection logic, will we be able to select a tree node by the item value rather than the MudTreeViewItem wrapper object itself? E.g., If my tree view was given a list of IReadOnlyCollection<Thing> _things;, can I say _myTree.Select(_things.First()), or something along those lines? I guess what I'm suggesting is that if we are passing in a collection (i.e., not defining MudTreeViewItem children explicitly), we won't have references to the MudTreeViewItem children such that we can pass one to a Select method. Therefore we'd need a way to select based on what we do know: an object from the collection we passed in.
• Do the new changes open the doors for things like automatic parent node expansion upon programmatically selecting a node? E.g., if I select a node a few levels deep, it could expand the parents above it to reveal its location.

Apologies if any of this was obvious from previous discussion, I haven't really had time to dive in yet. This weekend I plan to get versed in MudBlazor contribution so I can make better sense of this stuff. Thanks, and nice work!

With new selection logic, will we be able to select a tree node by the item value rather than the MudTreeViewItem wrapper object itself?

Yes. But not via Select( ), I made that internal but rather via @bind-SelectedValue. There isn't much difference. All you need to do is set the variable you bound to SelectedValue and call (treeview(IMudStateHasChanged)).StateHasChanged().

Do the new changes open the doors for things like automatic parent node expansion upon programmatically selecting a node?

This PR doesn't. But I think this is a very useful functionality and we can implement it fairly easily in a follow-up PR.

call ((IMudStateHasChanged)treeview).StateHasChanged()

That part should be unnecessary since Blazor should do it automatically.

_myTree.Select(...)

I think such public API should be prohibited in most cases, and it's good thing we removed them.
Developers should use two way binding for selection as henon mentioned.

@ScarletKuro

I think such public API should be prohibited in most cases

I would agree. Bit of an oversight on my part. This would be ideal for sure. Looking good!

call ((IMudStateHasChanged)treeview).StateHasChanged()

That part should be unnecessary since Blazor should do it automatically.

Of course, when the bound variable is set by another component via two-way binding or in an event handler of a blazor component. There are other cases (in a background thread, timer, etc), where you need to start a render manually in order to update the selected value after setting the variable you bound to SelectedValue.

• Do the new changes open the doors for things like automatic parent node expansion upon programmatically selecting a node? E.g., if I select a node a few levels deep, it could expand the parents above it to reveal its location.

@kelltom I just PRed auto-expansion #8762