microsoft / fluentui-blazor

Microsoft Fluent UI Blazor components library. For use with ASP.NET Core Blazor applications
https://www.fluentui-blazor.net
MIT License
3.77k stars 363 forks source link

fix: DialogService Result Data is always the same instance as the Input Data. (value types/string support?) #2365

Closed guimabdo closed 3 months ago

guimabdo commented 3 months ago

πŸ› Bug Report

DialogService Result Data always return the initial instance instead of the modified dialog Content.

πŸ’» Repro or Code Sample

πŸ€” Expected Behavior

In the sample above, the Result string Data should vary as the user inputs different text values into the FluentTextArea.

😯 Current Behavior

In the sample above, the Result string is always the same as the Input string ("Initial text")

πŸ”¦ Context

I'm trying to make a very simple textarea Dialog. For now, I'll proceed by making a class instance to wrap the string value.

🌍 Your Environment

dvoituron commented 3 months ago

That's how .NET works: string is a reference type but is immutable

Strings are immutable--the contents of a string object can't be changed after the object is created. For example, when you write this code, the compiler actually creates a new string object to hold the new sequence of characters, and that new object is assigned to b. The memory that had been allocated for b (when it contained the string "h") is then eligible for garbage collection. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/reference-types#the-string-type

So you can't use a Content attribute of type string but must pass an object.

MyDialog.razor

@implements IDialogContentComponent<DialogEditableExample.MyDialogClass>
<FluentTextArea @bind-Value="Content.Value" Immediate/>
@code {
    [Parameter]
    public DialogEditableExample.MyDialogClass Content { get; set; } = default!;
}

DialogEditableExample.razor

@inject IDialogService DialogService

<FluentButton OnClick="@EditAsync">Edit</FluentButton>

<p>Name: @MyDialogData.Value</p>

@code
{
    MyDialogClass MyDialogData = new() { Value = "Initial text" };

    private async Task EditAsync()
    {
        var dialogReference = await DialogService.ShowDialogAsync<MyDialog>(MyDialogData, new DialogParameters());
        var result = await dialogReference.Result;

        if (result.Data is MyDialogClass dialogData)
        {
            if (dialogData.Value == "Initial text") { throw new InvalidOperationException("Result should be the value inputed by the user..."); }
        }
    }

    public class MyDialogClass
    {
        public string Value { get; set; } = string.Empty;
    }
}

peek_6

guimabdo commented 3 months ago

@dvoituron I know that. I just feel that this service should somehow track the "Content" property value of an IDialogContentComponent, and use it when you're getting the .Result. It seems to me this would be the correct behavior.

I may try to contribute to this in the future If I eventually get some time...

dvoituron commented 3 months ago

I just feel that this service should somehow track the "Content" property value of an IDialogContentComponent.

I think that is the case, but because of the peculiarity of the string type, it's not. I think this problem will be the same for other "immutable" data types.

On the other hand, I find it more elegant to exchange objects between components: more scalable (among other things) 😊

guimabdo commented 3 months ago

On the other hand, I find it more elegant to exchange objects between components: more scalable (among other things) 😊

Yes, I think it might be more suitable in most cases.

Still, the current behavior makes me feel strange, as it makes it meaningless to obtain the result through result.Data.

In your example (and also on the documentation site) you already have the same reference in your MyDialogData property, there is no need to get it back from the result data.

Even for reference types, the component could have created a new instance internally (new MyDialogData()) and assigned it in the Content property, and perhaps this new reference should be the correct Result Data.