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

Nullable annotations for X11 #17814

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MrJul
Copy link
Member

@MrJul MrJul commented Dec 21, 2024

What does the pull request do?

This PR enables and fixes nullable annotations for the Avalonia.X11 project.

Remarks

While most of the changes are straightforward, X11Screens.Randr15ScreensImpl.CreateScreenFromKey() has been changed to return an empty FallBackScreen instead of null in case of failure. This code path is called from ScreensBase<TKey,TScreen>.EnsureScreens(), where null is definitely not expected, as that would insert a null screen into AllScreens, causing NREs:

var newScreen = CreateScreenFromKey(newScreenKey);

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054059-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

}
}

return null;
return new FallBackScreen(default, _x11);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's not a valid behavior to return a fake screen for invalid parameter here. It's a problem of the calling code to handle a missing screen properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of 10 Screen implementations, this one was the only one returning null - it's fairly recent, and should have respected the non-null contract. I guess it was missed due to the nullability mismatch here.

I've changed it to throw instead. We should never get here anyway: the key comes from the ScreenKeys property, and should always be found in the for loop above.

new X11PlatformOptions()));
{
var options = AvaloniaLocator.Current.GetService<X11PlatformOptions>() ?? new X11PlatformOptions();
_ = new AvaloniaX11Platform(options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a static Initialize method? Relying on ctor side effects outside of the class itself seems a bit out of place

Copy link
Member Author

@MrJul MrJul Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted to the old implementation and added null! initializers instead for now. We completely control the caller so it's fine.

The AvaloniaX11Platform instance registers itself inside the AvaloniaLocator currently, so replacing that with a static initialization method doesn't really help. Ideally we should refactor that code to completely decouple initialization and registration, but that's out of scope of this PR.

Copy link
Member

@kekekeks kekekeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general

@MrJul MrJul force-pushed the feature/nullable-x11 branch from f3a587f to 51efc8f Compare December 24, 2024 09:18
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0054079-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants