MudDialog: Add nullable annotation

[xC0dex]

Hey ✋,

I'll try to contribute to MudBlazor more often. This time with nullable annoations.

Description

Part of #6535

• Add nullable annotation for dialog related classes
• Introduced an internal static instance of DialogOptions and DialogParameters called Default to reduce some allocations

There is one thing to discuss:

The MudMessageBox defines the Title parameter as nullable. That's why the Show methods of the IDialogService now accept a nullable title because I would rather not suppress nullable here. But IMO, it would be better to not make the title nullable as the title will always be rendered right now in MudDialogInstance.
So we basically have to answer only one question: Should the Title be nullable or not?

How Has This Been Tested?

Added missing tests.

Type of Changes

• New feature (non-breaking change which adds functionality)

Checklist

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.

[ScarletKuro]

Non-breaking change which adds functionality

The problem is that on top of nullable, you do change the behavior:

MudBlazor/src/MudBlazor/Services/DialogService.cs

Line 310 in 877ccfd

public required string Title { get; set; }

This now says that the Title must always be supplied. If someone just calls new MessageBoxOptions(), they will get a compilation error. I understand it's the v7 branch where such changes are allowed, but below I'm explaining why some refactoring could be questionable for now.

MudBlazor/src/MudBlazor/Components/Dialog/MudDialogInstance.razor.cs

Line 52 in 877ccfd

public DialogOptions Options { get; set; } = new();

This also slightly changes the behavior. I'm not saying it's not a correct refactor—it is, and it's how it should have been—but I'd prefer to have parameter changes in a separate change, as we do with ParameterState.

The dialog is going to be refactored in the future, which is why I wouldn't do any refactor now. Just do a basic nullable annotation even if there will be clumsy spots, we can suppress it for now.
Also fyi regarding input controls #8447 (comment)

[ScarletKuro]

I'll try to contribute to MudBlazor more often. This time with nullable annoations.

I think @henon can invite you to the contribution team, it will be easier to communicate.

[henon]

Sure @xC0dex, please join the mudblazor discord server if you haven't already done so https://discord.gg/mudblazor
Then let me know your discord handle so I can add you to the contribution team channel.

[xC0dex]

Sure @xC0dex, please join the mudblazor discord server if you haven't already done so https://discord.gg/mudblazor Then let me know your discord handle so I can add you to the contribution team channel.

I already joined the server. My username is xc0dex.

[xC0dex]

Hey @ScarletKuro, thanks for the feedback!

If someone just calls new MessageBoxOptions(), they will get a compilation error

You're right. I didn't think about that. I'll revert that and initialize the title with null! .

I'm not saying it's not a correct refactor

I got your point. Usually when I spot a potential improvement, I fix it immediately. But I can understand, if this should be done in a separate PR 👍. Maintaining an open source project is another level of responsibility.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.82%. Comparing base (28bc599) to head (578e189).
Report is 281 commits behind head on dev.

❗ Current head 578e189 differs from pull request most recent head b44c17c

Please upload reports for the commit b44c17c to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #9146      +/-   ##
==========================================
+ Coverage   89.82%   90.82%   +0.99%     
==========================================
  Files         412      401      -11     
  Lines       11878    12516     +638     
  Branches     2364     2436      +72     
==========================================
+ Hits        10670    11368     +698     
+ Misses        681      608      -73     
- Partials      527      540      +13     

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

[xC0dex]

I'm a bit confused why codecov now started complaining

[henon]

Maybe the lines you touched weren't that well covered to begin with

[xC0dex]

Maybe the lines you touched weren't that well covered to begin with

Yeah, that's the reason. I was just wondering why it was not complaining when the PR was created.

[xC0dex]

I'll add the missing tests 👍

[xC0dex]

While writing the tests, I noticed that the Close method of the DialogService uses the implementation DialogReference instead of the interface IDialogReference. I think we could safely change this from:

public void Close(DialogReference dialog)

to

public void Close(IDialogReference dialog)

[ScarletKuro]

I think we could safely change this

I guess it should be safe from a binary compatibility viewpoint since it's not a virtual or abstract method.

[ScarletKuro]

Hmm, apparently the DialogResult can be null.

Before the annotation:

MudBlazor/src/MudBlazor/Components/Dialog/MudDialog.razor.cs

Line 233 in 61e25ab

public async Task CloseAsync(DialogResult result = null)

And this case was not covered by IDialogReference. The possibility of DialogResult being null makes things inconvenient. I think in the future we should reduce the number of nulls and use the null object pattern instead.

[henon]

Thanks Justin!