Make Display nullable in IWindow #6501
Conversation
SDL2 doesn't return bool for most of these functions, so it stays the same there.
|
Will fix tests soon... |
|
If you want to make it not crash, just ensure that the display is never set to null – in case there is no display, just lie to everyone that the last display is still current. Nullable display will complicated game code for no reason. |
|
I did what you suggested before trying this option, but it ended up with bunch of invalid display errors, so I decided to stop all operations regarding to Display until it becomes available again. |
| @@ -85,13 +85,13 @@ private void load(FrameworkConfigManager config, GameHost host) | |||
| AddStep("switch to borderless", () => windowMode.Value = WindowMode.Borderless); | |||
| AddAssert("check window position", () => new Point(window.Position.X, window.Position.Y) == display.Bounds.Location); | |||
| AddAssert("check window size", () => new Size(window.Size.Width, window.Size.Height) == display.Bounds.Size, desc2); | |||
| AddAssert("check current screen", () => window.CurrentDisplayBindable.Value.Index == display.Index); | |||
| AddAssert("check current screen", () => window.CurrentDisplayBindable.Value!.Index == display.Index); | |||
There was a problem hiding this comment.
Let this fail gracefully, use null forgiving operator. Ditto for all ! in tests.
| @@ -388,9 +391,9 @@ private static bool tryGetDisplayFromSDL(int displayIndex, [NotNullWhen(true)] o | |||
| /// <summary> | |||
| /// Gets the <see cref="Display"/> that has been set as "primary" or "default" in the operating system. | |||
| /// </summary> | |||
| public virtual Display PrimaryDisplay => Displays.First(); | |||
| public virtual Display? PrimaryDisplay => Displays.IsEmpty ? null : Displays.First(); | |||
There was a problem hiding this comment.
| public virtual Display? PrimaryDisplay => Displays.IsEmpty ? null : Displays.First(); | |
| public virtual Display? PrimaryDisplay => Displays.FirstOrDefault(); |
Ditto for SDL3.
| else | ||
| { | ||
| Logger.Log($"Failed to get window display bounds. SDL Error: {SDL_GetError()}"); | ||
| return Rectangle.Empty; |
There was a problem hiding this comment.
I'd rather this returns a non-empty rectangle. Maybe a 1x1 at (0, 0)?
SDL2 also needs this error handling.
| @@ -94,7 +94,7 @@ private bool mouseOutsideAllDisplays(Vector2 mousePosition) | |||
| break; | |||
|
|
|||
| default: | |||
| windowLocation = Host.Window.CurrentDisplayBindable.Value.Bounds.Location; | |||
| windowLocation = Host.Window.CurrentDisplayBindable.Value?.Bounds.Location ?? Host.Window.Position; | |||
There was a problem hiding this comment.
This breaks my mind. Does this fallback really work?
| @@ -170,12 +170,12 @@ public interface IWindow : IDisposable | |||
| /// <summary> | |||
| /// Gets the <see cref="Display"/> that has been set as "primary" or "default" in the operating system. | |||
There was a problem hiding this comment.
Needs to explain how/when it can be null.
|
Along with peppy's comment, I'll revisit after I understand a bit more how SDL or X11/Wayland is dealing with display disconnection. Considering that this only happens on Linux (so far), this needs more consideration. |
As explained in the discussion, it may be the case that all monitors are turned off and the window is left alone without any displays. Current osu!framework implementation expects there to be at least one display, and such expectation makes the game crash when there is no display available.
To fix the issue, this PR makes Display nullable in IWindow, and adds some checks to make it less unsafe. However, there still needs to be one display on launch. This PR only lets Display nullable later on.
I understand that making Display nullable sounds not so good, but I think it's less surprising than letting the game crash on a sudden monitor disconnection. I made a contact to the issue reporter, and confirmed that this PR fixes the issue.
Feel free to reject this PR if it doesn't feel right for you.
It's hard to add the test case since it doesn't seem to happen on Windows, and it would require manual intervention (turning off the monitor)... I'd like to hear if there is any idea for this.