MudTreeView: Fix selection issues

[jacob7395]

Description

closes #5704
closes #4492
closes #3394

This PR addresses issues #5704, #4492, #3394, and follows on from PR #5856. By tackling bind issues with IsSelected. Given the challenges of integrating async methods into property setters, I've introduced a new method:

public async Task SetSelectedValue(T value)

This method is designed to find a given value within the tree and execute the necessary selection logic. I'm seeking feedback on this approach, particularly regarding the following points:

Current usage of SelectedValue as an update event may be misleading due to the lack of a straightforward two-way bind. Would replacing this with a callback method and adding documentation for clarification be a best practice?

A new parameter Comparer was also added in the process.

How Has This Been Tested?

Unit tests are planned to cover these changes. I’m looking for feedback on the approach before proceeding with test implementation.

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)

The visual change is the same as in #5856.

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: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7ab417e) 87.18% compared to head (ca42ef6) 88.08%.
Report is 80 commits behind head on dev.

Files	Patch %	Lines
...MudBlazor/Components/TreeView/MudTreeView.razor.cs	95.65%	0 Missing and 1 partial ⚠️
...zor/Components/TreeView/MudTreeViewItemComparer.cs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #7810      +/-   ##
==========================================
+ Coverage   87.18%   88.08%   +0.90%     
==========================================
  Files         389      394       +5     
  Lines       11419    11770     +351     
  Branches     2299     2383      +84     
==========================================
+ Hits         9956    10368     +412     
+ Misses        966      874      -92     
- Partials      497      528      +31     

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

[henon]

@ScarletKuro What do you think about the addition of public async Task<bool> SetSelectedValue(T value) ?

[ScarletKuro]

Hi. Sorry for the delay.

@ScarletKuro What do you think about the addition of public async Task<bool> SetSelectedValue(T value) ?

If people ask for a method to select item from outside, then it makes sense.
This method should contain Async suffix - SetSelectedValueAsync

Also, shouldn't this method accept IEqualityComparer as well? What if I'm using a class that is not mine and I can't override the equals of the class?

[henon]

In MudChipset ...

MudBlazor/src/MudBlazor/Components/ChipSet/MudChipSet.razor.cs

Line 153 in 5ebb845

public IEqualityComparer<object> Comparer

... and others we have the possibility to set a comparer, so yes, it should also be added as a parameter to MudTreeView and then used for such comparisons. I was actually surprised that we have no Comparer parameter in tree view even though it uses a hashset internally.

[ScarletKuro]

It could be an optional parameter in the method, or an optional parameter in component itself, matter of taste.

[jacob7395]

I would lean towards an optional parameter but I can also see why having it in the component could be beneficial.

Happy with either way just let me know and I'll push a change.

[jacob7395]

Also when looking in some other places of the code base I saw code like this to deal with using asnyc in a property setter:

[Parameter]
        [Category(CategoryTypes.FormComponent.Data)]
        public DateTime? Date
        {
            get => _value;
            set => SetDateAsync(value, true).AndForget();
        }

I think a similar technique could be employed to resolve the issues in the setter but if done wrong could cause issues in peoples projects.

[henon]

It could be an optional parameter in the method, or an optional parameter in component itself, matter of taste.

It should be in the component IMO because it can be used for the internal hashset. So if the parameter is supplied it should in any case be also supplied to the hashset. You'd want consistent equality in the component everywhere.

[henon]

Also when looking in some other places of the code base I saw code like this to deal with using asnyc in a property setter:

[Parameter]
        [Category(CategoryTypes.FormComponent.Data)]
        public DateTime? Date
        {
            get => _value;
            set => SetDateAsync(value, true).AndForget();
        }

I think a similar technique could be employed to resolve the issues in the setter but if done wrong could cause issues in peoples projects.

This is not the only way. You could also use SetParametersAsync to call the setter and await it and it is probabably the better way of doing it. We only adopted this pattern very recently.

[ScarletKuro]

Also when looking in some other places of the code base I saw code like this to deal with using asnyc in a property setter:

        [Parameter]
        [Category(CategoryTypes.FormComponent.Data)]
        public DateTime? Date
        {
            get => _value;
            set => SetDateAsync(value, true).AndForget();
        }

I think a similar technique could be employed to resolve the issues in the setter but if done wrong could cause issues in peoples projects.

This technique is not what we would like to use, since we want to get rid of it in future.

[jacob7395]

Okay, so won't do the async in setter.

Can you point me to an example of using SetParametersAsync to achieve this.

Also trying to look this up online has lead me to a couple of stack overflow posts saying don't use SetParametersAsync, I would like to understand why I'm trying to learn more about blazor

https://stackoverflow.com/questions/76475515/should-i-be-using-setparametersasync-for-all-parameter-properties

[henon]

The argument against using SetParametersAsync is based around optimization. We want to achieve a different goal by using it, namely to await async code that needs to be executed after the parameter has been set. We want to await the code because of error handling which is built into await. By using .AndForget(), which we did a lot in the past, it is possible that exceptions get lost which can cause the user headaches.

Here is an example from another PR:

[jacob7395]

In the example you gave would this mean Hidden was set evetime any other parameter in the component was set, this might be what Blazor is doing anyway. I guess the value wont change until the base is called so you could do a diff with value returned from
SetParamaters.

Also the post I referenced talked about ParameterView going stale if you await in the method?

Either way I'll have a play around tomorrow morning and see what I come up with.

[henon]

If you have to await more than one method because more than one parameter changed you could just add the tasks into a list and await them with Task.WhenAll or so

[jacob7395]

@henon and @ScarletKuro made a few changes, but it did trigger a few more questions.

The code I've commit works with my testing, when I set SelectedValue in my app it gets set as you would expect, but how should this interact with the MudTreeViewItem selected item.

If someone manually changes the selected value of a tree item should we try and listed for the changes and reflect that in the SelectedValue.

There is also a question about what to do it someone set selected value to an invalid option, not figured out a nice way to handle this.

I've also not done the compare part yet.

[henon]

If someone manually changes the selected value of a tree item should we try and listed for the changes and reflect that in the SelectedValue.

I don't know much about treeview as it was coded by someone else but in any case the internal state of the component should remain consistent no matter which method was used to manipulate it. I hope that helps you with this question.

[henon]

There is also a question about what to do it someone set selected value to an invalid option, not figured out a nice way to handle this.

I think the selected value should then be null or default(T)?

[jacob7395]

@henon and @ScarletKuro I think everything is done now, take a look when you have time.

[jacob7395]

@henon I've added a test to cover the most of the code I've written the only no covered code is a guard clause that I don't think it's possible to trigger but catches a possible null issues.

Is this okay to go in, if so how? not too sure on the process yet.

[henon]

Good job @jacob7395, thank you very much for your efforts!

[henon]

And to answer your question, yes, sometimes some errorhandling statements can not be tested without extreme efforts. That is OK. It is not worth the effort mostly.

[jacob7395]

Thank you both for your help, I learned a lot from this one. The stuff around not using property setters for parameters is really interesting.

[gabephudson]

I deleted my comments from earlier today and was able to determine it was a visibility/async issue on my end causing this enhancement to break my existing code.

Binding the SelectedValue now works great and highlights the node! Thanks for the long-needed enhancement! I was able to remove all my "work around" code for this issue. :)

[kelltom]

I'm guessing this resulted in the Select method being added in... Is there any example usage of this? I'm wondering how it would be used in the case where we can't identify the MudTreeViewItem object to select.

In this example, I feed the MudTreeView a hash set of <TreeItemData>.

        <MudTreeView @ref="_mudTree" T="TreeItemData" Items="_treeItems" SelectedValue="_selectedItem" SelectedValueChanged="OnSelectedItemChanged">
            <ItemTemplate>
                <MudTreeViewItem  Value="@context" Text="@context.Name" />
            </ItemTemplate>
        </MudTreeView>

    private HashSet<TreeItemData> _treeItems = new HashSet<TreeItemData>();
    public class TreeItemData
    {
        public MyObject MyObject { get; set; }
        public HashSet<TreeItemData> TreeItems { get; set; } = new HashSet<TreeItemData>();

        public TreeItemData(MyObject myObject)
        {
            MyObject = myObject;
        }
    }

I'm having trouble programatically setting the _selectedItem. Setting it directly to the TreeItemData object from the Items list doesn't seem to work.

[ScarletKuro]

I'm guessing this resulted in the Select method being added in... Is there any example usage of this?

You mean the SetSelectedValue(T? value)?
You can't, it's internal, also this changes gives some headache #8360

[kelltom]

You mean the SetSelectedValue(T? value)?

There's an exposed Select method:

public async Task Select(MudTreeViewItem<T> item, bool isSelected = true)
{
    if (MultiSelection)
    {
        await item.Select(isSelected);
        await SelectedValuesChanged.InvokeAsync(SelectedValues);
        return;
    }

    if (isSelected)
    {
        await SetSelectedValue(item.Value);
        return;
    }

    await SetSelectedValue(default);
}

This looks like what we want, but it's asking for a MudTreeViewItem. From my MudTreeView reference, I don't know how to access its internal MudTreeViewItem collection such that I can identify the one I'd like to programtically select.

My scenario is this:

• User navigates to my site with query parameter: scheme://host/?objectId=1234
• My page loads all of the "objects" from database, converts them into a HashSet<TreeItemData> such that I can feed it into my MudTreeView.
• Once parameters set, I want to select the tree item that has id 1234 such that it is highlighted and appears to have been clicked.

[ScarletKuro]

There's an exposed Select method. Is there any example usage of this?

You can look the code, for example the tests, one of the examples:

<MudTreeView @ref="_treeView" T="string" @bind-SelectedValue="selectedValue">
    <MudTreeViewItem @ref="_item1" Value='"content"' />
    <MudTreeViewItem @ref="_item2" Value='"src"' />
</MudTreeView>

@code {
    MudTreeView<string> _treeView;
    MudTreeViewItem<string> _item1;
    MudTreeViewItem<string> _item2;

    public string selectedValue;

    public async Task SelectFirst()
    {
        await _treeView.Select(_item1);
    }

    public async Task SelectSecond()
    {
        await _treeView.Select(_item2);
    }

    public async Task DeselectSecond()
    {
        await _treeView.Select(_item2, false);
    }

}

[henon]

In MudChipSet and MudList we just removed selection via chips and list items and moved to a completely data-driven selection via SelectedValue. I guess we should make this change for MudTreeView as well

[ScarletKuro]

In MudChipSet and MudList we just removed selection via chips and list items and moved to a completely data-driven selection via SelectedValue. I guess we should make this change for MudTreeView as well

We should, but i feel like TreeView requires 90% code drop, it's just all way around wrong.
All of its code is using somehow stale values and writing directly to parameters.
While I could successfully move "Expanded", "Selected" paramters to ParameterState, the rest when I try is just going crazy, like the "Activated" tho the code doesn't look complicated, the methods SetSelectedValue, Select, SelectItem are just really weird, it's hard to debug all this.

[henon]

I'll rewrite it completely when I'm done with MudList where I want to do some small improvements. This component was contributed by a non-active member anyway, so it is about time we consolidate it.

[ScarletKuro]

I can post the Expanded and Selected proerties change if u want

[henon]

How about you just PR it and I continue from there?

[kelltom]

Thanks for looking into all of this. If you are going to rewrite, I'd also like to mention that virtualization is essential for this type of component. As it stands, if you expand too many nodes it'll bog down your app.

[henon]

@kelltom sure, but that'll be a separate work item and if you could help with that (once we got the breaking changes done in v7) that would be great.

[henon]

@kelltom I just found out that MudTreeView 's selection logic works perfectly find if you just use @bind-SelectedValue. Here is proof on try.mud

No need to use this: public async Task Select(MudTreeViewItem<T> item, bool isSelected = true) and I am inclined to remove it and just make sure multi-selection works well with an IReadOnlyCollection<T> instead of a HashSet<T>.

[henon]

Oh, sorry, it doesn't work when you initialize the selected value in the bound variable. I will fix that.