Rewrite SDL2 windowing logic to better handle changes from outside and to avoid feedback loops

[Susko3]

RFC

My general idea of what the finished flow would look like:

public partial class SDL2DesktopWindow : IWindow
{
    private Display currentDisplay; // kinda sucks that there need to be this many "current displays", but alas.

    protected Display CurrentDisplay // protected so the name doesn't clash with the above
    {
        get => currentDisplay;
        // this could maybe be a function `setCurrentDisplay()`?
        // this avoids having a redundant property, one that may sometimes clash with required `IWindow` properties.
        set => CurrentDisplayBindable.Value = currentDisplay = value;
    }

    public Bindable<Display> CurrentDisplayBindable = new Bindable<Display>();

    public void SetupWindow(FrameworkConfigManager config)
    {
        // these BindValueChanged should be simple, the bulk of the logic should be in the likes of `updateDisplay`
        CurrentDisplayBindable.BindValueChanged(display =>
        {
            if (display.NewValue == CurrentDisplay)
                // if the values match, that means that the this set operation originates from within SDL2DesktopWindow
                // updating the display would lead to a feedback loop.
                return;

            Debug.Assert(display.OldValue == CurrentDisplay) // should probably pass, but this might not always be true

            ScheduleCommand(updateDisplay, display.NewValue.Index);
        });
    }

    private void updateDisplay(int displayIndex)
    {
        SDL.SDL_SetWindowPosition(...);

        // importantly, we always query SDL when setting things from foreign requests, so that the values are up-to-date.
        // notice that changing CurrentDisplayBindable.Value will _never_ set CurrentDisplay directly.
        // always assume that the foreign value may be invalid and that SDL might not be able to set the state as requested.
        CurrentDisplay = SDL.SDL_GetCurrentDisplay(SDLWindowHandle);

        // possibly update other things that may change when the display has changed,
        // or let the appropriate SDL_Events arrive (preferred)
    }

    private void handleDisplayEvent(SDL.SDL_DisplayEvent e)
    {
        // it's important that the code that interacts with SDL doesn't have to deal with bindables/flow/whatever
        // and can simply just set the properties.
        CurrentDisplay = SDL.SDL_GetCurrentDisplay(SDLWindowHandle);
    }
}

Some thoughts on public window properties

From IWindow:

Read-write:

WindowState WindowState { get; set; }

This includes minimized/maximized and seems to work well in updateWindowStateAndSize()

Bindable<WindowMode> WindowMode { get; }

This one doesn't have minimized/maximized. Treated as an alias to WindowState.

Bindable<Display> CurrentDisplayBindable { get; }

Should be extra careful about feedback loops.

Size MinSize/MaxSize { get; set; }

This works well.

Read-only

IEnumerable<Display> Displays { get; }

Already handled well with #5439.
The CurrentDisplay should also update when the displays change (to have latest resolution, etc.)

Display PrimaryDisplay { get; }

Basically Displays.First(), could be removed in favour of CurrentDisplayBindable.Default.

IBindable<DisplayMode> CurrentDisplayMode { get; }

I find it odd that this is not a read-write Bindable - having this writeable should allow setting the refresh rate in addition to resolution (currently via FrameworkSetting.SizeFullscreen). Maybe a FrameworkSetting.FullscreenRefreshRate should be available, or FrameworkSetting.SizeFullscreen being size+refresh rate.

Size ClientSize { get; }

This seems to work fine.

public event Action? Resized;

This one is fine.

From FrameworkConfigManager / FrameworkSetting

Read-write

FrameworkSetting.WindowedSize / FrameworkSetting.WindowedPositionX / FrameworkSetting.WindowedPositionY

The window should actually respect these on startup, in SDL_CreateWindow (Currently, the window's position and size are updated a bit later).

FrameworkSetting.LastDisplayDevice

Should be respected on startup.
Should be an alias to setting CurrentDisplayBindable or vice-versa.

FrameworkSetting.WindowMode

Basically an alias to Bindable<WindowMode> IWindow.WindowMode.

Write-only

FrameworkSetting.SizeFullscreen

Could be expanded to also have a refresh rate.
This could be changed to read-write: the window will update the value with the current fullscreen resolution. Need to consider moving window between monitors (probably best to reset to default in that case).

From SDL2DesktopWindow public properties

Read-write

public Point Position { get; set; }

This should be private, windowed position is to be set via FrameworkSetting.WindowedPositionX/Y.
Or be get-only, with it being valid in windowed and fullscreen.

public float Scale;

Should be internal. And definitely private set;

Read-only

public virtual Size Size { get; protected set; }

Should be protected, as ClientSize is preferred. Also doesn't seem to do anything useful, instead only complicates the flow.

public Display CurrentDisplay { get; private set; }

Shouldn't exist, alias to CurrentDisplayBindable

public event Action<WindowState>? WindowStateChanged;

Could change WindowState to be a bindable. But this works fine for now.

public event Action<Point>? Moved;

Unused, can be removed.

[frenzibyte]

Looking at the code snippet you have for "current display" flow, I've had to re-read a few times to understand how it should work. Documentation and better names would definitely make it read better (i.e. CurrentDisplayBindable should just be named CurrentDisplay, and the existing private field should be named windowCurrentDisplay or internalCurrentDisplay):

public partial class SDL2DesktopWindow : IWindow
{
    /// <summary>
    /// The display the window is placed on currently, as reported by SDL.
    /// This is always sourced from the SDL display query method.
    /// </summary>
    private Display windowCurrentDisplay;

    /// <summary>
    /// The display the window is placed on currently.
    /// </summary>
    public Bindable<Display> CurrentDisplay = new Bindable<Display>();

    public void SetupWindow(FrameworkConfigManager config)
    {
        // these BindValueChanged should be simple, the bulk of the logic should be in the likes of `updateDisplay`
        CurrentDisplay.BindValueChanged(display =>
        {
            if (display.NewValue == windowCurrentDisplay)
                // if the values match, that means that the this set operation originates from within SDL2DesktopWindow
                // updating the display would lead to a feedback loop.
                return;

            Debug.Assert(display.OldValue == CurrentDisplay) // should probably pass, but this might not always be true

            ScheduleCommand(updateDisplay, display.NewValue.Index);
        });
    }

    private void updateDisplay(int displayIndex)
    {
        SDL.SDL_SetWindowPosition(...);

        // importantly, we always query SDL when setting things from foreign requests, so that the values are up-to-date.
        // notice that changing CurrentDisplayBindable.Value will _never_ set CurrentDisplay directly.
        // always assume that the foreign value may be invalid and that SDL might not be able to set the state as requested.
        refetchCurrentDisplay();        

        // possibly update other things that may change when the display has changed,
        // or let the appropriate SDL_Events arrive (preferred)
    }

    private void handleDisplayEvent(SDL.SDL_DisplayEvent e)
    {
        // it's important that the code that interacts with SDL doesn't have to deal with bindables/flow/whatever
        // and can simply just set the properties.
        refetchCurrentDisplay();
    }

    private void refetchCurrentDisplay()
    {
        windowCurrentDisplay = SDL.SDL_GetCurrentDisplay(SDLWindowHandle);
        CurrentDisplay.Value = windowCurrentDisplay;
    }
}

IBindable CurrentDisplayMode { get; }

I find it odd that this is not a read-write Bindable - having this writeable should allow setting the refresh rate in addition to resolution (currently via FrameworkSetting.SizeFullscreen). Maybe a FrameworkSetting.FullscreenRefreshRate should be available, or FrameworkSetting.SizeFullscreen being size+refresh rate.

I would prefer separate setting, rather than complicating things with a struct value for one setting.

public virtual Size Size { get; protected set; }

Should be protected, as ClientSize is preferred. Also doesn't seem to do anything useful, instead only complicates the flow.

As far as I recall back in OsuTKWindow, this used to provide the size in window space (post-DPI-scale), while ClientSize would provide the size in drawable space (pre-DPI-scale).

public Point Position { get; set; }

This should be private, windowed position is to be set via FrameworkSetting.WindowedPositionX/Y.
Or be get-only, with it being valid in windowed and fullscreen.

I'm generally confused about whether something should be exposed in the IWindow or hooked up to a FrameworkSetting and changed from there, it's not really quite defined. I think FrameworkSettings should only be defined to save states that would be loaded on next session, rather than completely removing window properties in favour of it. Not sure.

[Susko3]

Agree on your naming. Will use internal to avoid windowWindowMode. Also, I was looking for a verb for what refetchCurrentDisplay() does and (re)fetch works perfectly here!

Open for suggestions for a better name for updateDisplay() that makes it clear that the update is a request from the outside that will change SDL state. Maybe requestDisplayChange(), not sure.

[Susko3]

The update flow described here is a bit flawed. I originally envisioned it as moving the window to the specified display while keeping all other things (eg. WindowMode) at their current value. The problem happens when multiple things change at the same time. While that is unlikely once the game is running, the startup case is one that needs to be handled carefully.

I've opted to expand the concept of pendingWindowMode, to also have pendingDisplayMode. This works well in the startup case. Currently, with #5528, it's still a bit weird that pendingWindowMode and pendingDisplayMode are set in two different places during startup.

Something to think about in future code cleanups.