Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TreeView: SelectedNodeChanged-Event is fired when there should be no change #4413

Closed
WolfgangKluge opened this issue Dec 22, 2022 · 5 comments
Assignees
Labels
Type: Bug 🐞 Something isn't working
Milestone

Comments

@WolfgangKluge
Copy link
Contributor

WolfgangKluge commented Dec 22, 2022

Describe the bug
I have a TreeView and whenever the user want's to change the current node, I want to ask the user before taking any action.

To Reproduce
Here is a simple example

<TreeView TNode="string" Nodes="nodes" SelectedNode="selectedNode" SelectedNodeChanged="SelectedNodeChanged">
    <NodeContent>@context</NodeContent>
</TreeView>
Selected: @selectedNode
@code {
    [Inject] IMessageService MessageService { get; set; } = default!;

    string?[] nodes = new[] { "A", "B", "C" };
    string? selectedNode = "A";

    private async Task SelectedNodeChanged(string? newSelectedNode)
    {
        // is fired twice (so fast you'll never see the first message)
        if (!await MessageService.Confirm($"Navigate from {selectedNode} to {newSelectedNode}?")) return;
        selectedNode = newSelectedNode;
    }

    private async Task SelectedNodeChangedSwitchBack(string? newSelectedNode)
    {
        var prevNode = selectedNode;
        selectedNode = newSelectedNode;
        if (!await MessageService.Confirm($"Navigate from {prevNode} to {newSelectedNode}?")) {
            selectedNode = prevNode; // fires the event again (since it differs from the `state.SelectedNode`)
        }
    }
}

Whenever you click on any other node than the selected, a message pops up.

I expect the message shown states something like Navigate from A to B? and if you debug that, there is really such a message - but right after the first message, there's a second one stating Navigate from A to A?. At the end nothing happens.

If you use SelectedNodeChangedSwitchBack, the behavior is somewhat different and it seems to work - but then you get two messages in a row.

I think, the main reason for the whole problem is, that you invoke SelectedNodeChanged whenever SelectedNode is changed - even if that change comes from outside.

public TNode SelectedNode
{
get => state.SelectedNode;
set
{
if ( state.SelectedNode.IsEqual( value ) )
return;
state.SelectedNode = value;
SelectedNodeChanged.InvokeAsync( state.SelectedNode );
DirtyClasses();
}
}
, especially the line SelectedNodeChanged.InvokeAsync( state.SelectedNode );

If I move that logic (except DirtyClasses) to SelectNode I get the expected behavior.

Expected behavior
I expect to change selectedNode without getting SelectedNodeChanged fired.

You should only fire events, if you are changing the values (not when the value is changed from outside your component). InvokeAsync inside the property setter of [Parameters] seems no good idea (for all components, not only the TreeView).
dotnet/aspnetcore#26230

Additional context
My workaround is to test, if the newSelectedNode is changed at all - but this seems unexpected and unnecessary.

@WolfgangKluge WolfgangKluge added the Type: Bug 🐞 Something isn't working label Dec 22, 2022
@WolfgangKluge
Copy link
Contributor Author

WolfgangKluge commented Dec 22, 2022

Maybe the same like #2380 (but it depends on the implementation of the custom SelectedNodeChanged)

@David-Moreira
Copy link
Contributor

Thanks for additionally noting this:
image
We've actually just touched upon this recently, we are thinking of changing and centralizing what needs changing on SetParametersAsync with the actual async support, and according to the provided Blazor guidelines. I'll go ahead and open a refactoring issue right now.

@David-Moreira
Copy link
Contributor

@stsrki did you fix this on your Tree View Changes PR?

@stsrki
Copy link
Collaborator

stsrki commented Jan 26, 2023

I have tested @WolfgangKluge in the new version, and the new TreeView changes work. That is, it shows only one dialog. I assume that is what is needed?

One thing that still doesn't work is when the user clicks on Cancel button, the last selected node still stays with the selected background color. I will try to fix that before release, otherwise, it will have to wait for a patch.

@stsrki
Copy link
Collaborator

stsrki commented Feb 12, 2023

@WolfgangKluge any chance you could clone the repository and test the fixes in the rel-1.2-treeview-selectednode-fixes branch?

@stsrki stsrki closed this as completed Feb 17, 2023
@stsrki stsrki added this to Support Aug 3, 2024
@stsrki stsrki moved this to ✔ Done in Support Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug 🐞 Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants