Fix passing parent_window=None to message_box

[Matiiss]

Currently passing parent_window=None to message_box raises a TypeError. This should fix that.
What exactly does the parent_window even do? The SDL docs don't really explain it either from what I could find. The only notable thing was that message_box should be called from the thread that the parent window was created in, unless it's None, in which case, it should be called from the main thread. Should that be added to the docs as well? Can that parent_window param in the docs be uncommented now?
Should the test be an interactive one instead?

[Matiiss]

Well, I guess the CI really doesn't like multiprocessing, going to move the test to the interactive tests section.

[Starbuck5]

What exactly does the parent_window even do? The SDL docs don't really explain it either from what I could find

Perhaps this should be a part of the research process for this PR?

[Matiiss]

What exactly does the parent_window even do? The SDL docs don't really explain it either from what I could find

Perhaps this should be a part of the research process for this PR?

Well, in my understanding all this does is sort out which thread the messagebox will be opened in. That is, if you may not wish to block your process running on the main thread, you could create a new window in a child thread and then pass that window as the parent to the message box function (that you also probably need to call from that thread). Otherwise even if you call it in a thread, but don't specify a parent window, it will block the main thread.
That's what I understand from the docs

This function should be called on the thread that created the parent window, or on the main thread if the messagebox has no parent. It will block execution of that thread until the user clicks a button or closes the messagebox.

https://wiki.libsdl.org/SDL2/SDL_ShowSimpleMessageBox

There's a slight issue, unrelated to this PR. Something must be broken in the implementation because this does not work the way I have explained (and believe it should be the way it should work) with our implementation. It's beyond the scope of this PR though.

[robertpfeiffer]

What exactly does the parent_window even do? The SDL docs don't really explain it either from what I could find

Perhaps this should be a part of the research process for this PR?

As I understand it, setting the parent window of a modal dialog makes it so the window is greyed out until the modal dialog is closed/dealt with. It's not a thing the toolkit does (SDL, GTK, Qt), but something that must be actively supported by the desktop environment/window manager/compositor. The window manager might decide to always focus automatically on the dialog when you alt-tab to the parent window or click in the talk bar, to group the dialog together with the parent window in the dock/task bar, to grey out the parent window, and so on. If there is no parent window set, a tiling window manager might just show the dialog centered and on top of the tiling area. Otherwise it could show the window on top of the parent window.

Given that wayland does not allow windows to position themselves, this information is needed for the dialog to be spawned on top of the application and on the right monitor.

[robertpfeiffer]

The thing you should take away from my previous comment is it depends on whether you are on Win32, Quartz, X11, Wayland, or Android. Consoles that don't have a classic "window manager" might still have limited support for modal dialogs, the same way and Android and iOS do.

[MyreMylar]

This is what parent Window does on Windows (from SDL source):

/* If we have a parent window, get the Instance and HWND for them
       so that our little dialog gets exclusive focus at all times. */
    if (messageboxdata->window) {
        ParentWindow = messageboxdata->window->driverdata->hwnd;
    }

And from a Microsoft blog on the attribute:

The hwndParent field stores a handle to the parent window. This allows the resulting dialog to behave as a modal window and optionally lets you position the window relative to the parent.

Looking at the SDL code it looks like they don't touch the 'position the window relative to the parent' flag so unless it is on by default for this window type you would just get the modal effect from setting the parent window on Windows.

[MyreMylar]

LGTM 👍

[zoldalma999]

This test seems to check the opposite of what the pr fixes (checks for typeerror when passing parent_window=None), but happens to pass because buttons being empty is a typeerror.

[Matiiss]

The difficulty in testing this is that it was previously failing where it shouldn't have failed at all and now it doesn't fail, but it can't exactly be tested for success because the function is blocking. Initially I attempted a hackish solution using multiprocessing to circumvent this issue, however, that failed on CI and as I said, it was hackish. So, I moved the blocking part to the interactive tests section so it can be tested interactively. Now thinking about it, this doesn't guarantee that the empty buttons is the cause of the TypeError... I could make it raise say the IndexError for buttons while passing the parent_window=None because the parent_window is internally checked first, so, if it raises an IndexError, it successfully passed the window checks, thus parent_window would not be the cause of any errors.

Now, the better approach would be to test this using a mock SDL_MessageBox function in a C test suite and then you can just call the pg_message_box and ensure that it returns what it should return instead of NULL, but we don't have a C test suite (yet).

So yeah, I would propose making it error on the buttons with an IndexError and make sure that the IndexError is raised and parent_window=None, that's really the best way I can think of testing for success.

[zoldalma999]

I see, that makes sense. I think changing the error type to IndexError is not the way to go (it is a public api, plus IndexError is not really the proper error here). I am fine with just removing this test, since it is in an interactive test suite as well.