[GH-3544] Add null check for Application.Current #3546
Conversation
| var appStyle = (window.IsNull() | ||
| Theme appStyle; | ||
|
|
||
| if (Application.Current.IsNull()) // In case the Window is hosted in a WinForm app |
There was a problem hiding this comment.
I have used the IsNull() extension method here as it seems to be used in this class. If needed I could extract the methods IsNull() and IsNotNull() into a separate class and use them for the other changes.
There was a problem hiding this comment.
I don't know what is better in these cases. Should we always use these 2 extension methods? Or not? I don't know...
There was a problem hiding this comment.
TBH I am not really sure to understand what those 2 methods bring compare to having the null check. Was it to avoid some FPs from ReSharper with the null reference rules?
There was a problem hiding this comment.
Yes this could be the reason, @batzen can explain this a little bit more.
There was a problem hiding this comment.
I added those because writing if (!(xy is null)) feels horrible, whereas if (xy.IsNotNull()) does not. And to be consistent i also added IsNull.
There was a problem hiding this comment.
@batzen I don't know whether this is a good practice or not but you could use if (x is object) to test non-nullability.
There was a problem hiding this comment.
just to complete this: Since C# 7.0 you could also use this code:
if (SomeObject is SomeType variableName)
{
// Do some stuff with variableName
return variableName.ToString();
}
More about this: https://docs.microsoft.com/dotnet/csharp/language-reference/keywords/is
Happy coding
Tim
There was a problem hiding this comment.
@timunie That's correct, and I use it more and more, because of better readable code, but sometimes you don't need extra vars ;-)
|
Would you mind adding your changes/fixes to ThemeManager.cs also to Fluent.Ribbon? |
Add null checks for usage of Application.Current in the main part of the library.
punker76
force-pushed
the
app-current-null
branch
from
1e91a23
to
5876968
Compare
|
@Evangelink your changes have been merged, thanks for your contribution 👍 |
|
@batzen I'll do the PR on |
Describe the changes you have made to improve this project
Add null checks for usage of
Application.Currentin the main part of the library. Note that the demo and the unit tests haven't been updated as they are unlikely to be used wrapped in a WinForm App.Unit test
No unit test added but if needed I can add a new test project with a WinForm app hosting the WPF MahApps window.
Additional context
Closed Issues
Fix #3544