Added unit tests for display.toggle_fullscreen and display.get_active

[tsadama]

Hello, this pull request addresses #1743 and #1755.

The unit tests created for display.toggle_fullscreen() and display.get_active() were created with stipulations from the documentation and from my understanding of the code. I used pylint to check my code.

Two things I noticed when testing locally:

• The documentation for display.get_active() states that it will return false when an image is iconified. However, iconify returned true, but get_active returned true still.

• When running display.set_mode() with width/height of (0,0) and then display.toggle_fullscreen(), the resulting width/height becomes (800, 600) with a display.toggle_fullscreen() return value that is not 1. However for all other width/heights I tried, the resulting width/height is the same as the original with a display.toggle_fullscreen() return value that is not 1.

Please let me know if I should make any changes!

[tsadama]

Follow up: it looks like one of the reasons for the unsuccessful build is because of the bullet point number 1. Guidance on how I should address this would be appreciated! I will address the other errors soon!

[MyreMylar]

Hello, this pull request addresses #1743 and #1755.

The unit tests created for display.toggle_fullscreen() and display.get_active() were created with stipulations from the documentation and from my understanding of the code. I used pylint to check my code.

Two things I noticed when testing locally:

• The documentation for display.get_active() states that it will return false when an image is iconified. However, iconify returned true, but get_active returned true still.

Since this is happening locally and nor just on CI it sounds like a bug with get_active(). One of the issues we are trying to solve with getting unit tests for everything is finding actual bugs lurking in the code. I will have a poke at this tomorrow.

Sometimes the Continuous Integration builds behave differently to local builds.

• When running display.set_mode() with width/height of (0,0) and then display.toggle_fullscreen(), the resulting width/height becomes (800, 600) with a display.toggle_fullscreen() return value that is not 1. However for all other width/heights I tried, the resulting width/height is the same as the original with a display.toggle_fullscreen() return value that is not 1.

Interesting, I guess it can't really make a full screen display with 0 pixels to play with so picking a reasonable default seems OK to me?

[tsadama]

Thank you for the response. In the meantime, I will fix some of the other errors that popped up with the CI. I will keep it on my local branch until I hear back about get_active(). Thanks again!

[MyreMylar]

I had a quick look at get_active() and remembered this old bug:

#161

Which, reading between the lines, seems to indicate that get_active() maybe doesn't work only on windows?

Are you on windows @tsadama ?

[MyreMylar]

I'm using this MOR from the old bug:

import pygame


pygame.init()
window_surface = pygame.display.set_mode((640, 480))

is_running = True
while is_running:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            is_running = False

    if not pygame.display.get_active():
        print(str(pygame.display.get_active()))

    pygame.display.update()

Last time @illume reported that it worked on mac and @nthykier reported that it worked on his machine, which I'm guessing is probably linux of some flavour?

[MyreMylar]

Tried that same code on Raspberry Pi OS (which is basically Debian) and it works fine. So it's looking like it is probably a windows issue.

[MyreMylar]

Poked at the C code some, it looks like on windows SDL_WINDOW_SHOWN is always true whatever the minimised state of the window. However, I notice that there is also another flag called SDL_WINDOW_MINIMIZED which does work on windows.

Perhaps you could change the appropriate lines of display.c to read something like:

#if IS_SDLv2
static PyObject *
pg_get_active(PyObject *self, PyObject *args)
{
    Uint32 flags = SDL_GetWindowFlags(pg_GetDefaultWindow());
    return PyBool_FromLong((flags & SDL_WINDOW_SHOWN) &&
                           !(flags & SDL_WINDOW_MINIMIZED));
}
#else  /* IS_SDLv1 */
static PyObject *
pg_get_active(PyObject *self, PyObject *args)
{
    return PyBool_FromLong((SDL_GetAppState() & SDL_APPACTIVE) != 0);
}
#endif /* IS_SDLv1 */

And see if it makes your tests work locally at least? Not sure how comfortable you are with editing C code and compiling pygame.

[tsadama]

Hi @MyreMylar , I am actually using Ubuntu 16.04. I tried the MOR a few posts above, and my command line is being flooded with False so it seems like it's working.

In terms of the change to the C code, I applied it and then did the following:
python3 setup.py build
sudo python3 setup.py install
ran the unit test on get_active

I still got the output where get_active would return true after a call to iconify, although I'm not entirely sure that I am rebuilding/recompiling it properly after changing the C code.

Update: curiously, I am also getting a get_active error from the Travis CI from python 3.5 where line 73 assert fails. Basically this assert is right after a pygame.display.quit() to check that we get a false initially. It seems like all other fails are from the iconify issue mentioned above. Also please ignore the third commit below as it was a mistake that I made and fixed.

[MyreMylar]

Not looked over what you've done yet, but I wonder if a couple of calls to pygame.event.get() or pygame.event.pump() after calling iconify() will sort out the issue for the unit tests, at least locally. I suspect that it might be the sort of SDL instruction where it sends a request to the OS and then waits for an event to come back to know if it worked.

On the CI you may always have a problem with the 'dummy' video driver they use there, that causes many display related test problems.

[tsadama]

Follow up:
I've tried modifying the C code as mentioned above and I've also tried updating the display, and using calls to pygame.event.get() and pygame.event.pump(). Unfortunately my local tests still fail, and I am unable to determine why iconify does not cause get_active() to be false even though manually minimizing the screen works. Could it be possible that iconify is buggy here? If so, it might be worth saving the iconify/get_active() check for an iconify unit test.

[MyreMylar]

Could it be possible that iconify is buggy here? If so, it might be worth saving the >iconify/get_active() check for an iconify unit test.

I guess an iconify() test case would be useful to check that. Modifying the code above:

import pygame


pygame.init()
window_surface = pygame.display.set_mode((640, 480))

is_running = True
while is_running:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            is_running = False
        if event.type == pygame.KEYDOWN:
            pygame.display.iconify()

    if not pygame.display.get_active():
        print(str(pygame.display.get_active()))

    pygame.display.update()

For me, on windows & Rasperry Pi, that makes pressing any key minimize the window just as if I'd pressed the minimize button on the menu bar. It doesn't print False when minimized on dev10, but it does if I rebuild pygame adding the C code change mentioned above.

If pygame.display.iconify() doesn't minimize the window on linux Ubuntu then I guess we have our answer. If it does, but doesn't print False then it's worth trying the example above again with the C code change that works on windows. If that still doesn't work then I guess something screwy is happening.

[MyreMylar]

Just tried the above code using pygame.display.iconify() and it works fine on Raspberry Pi OS (which is Debian based) and using dev10. It prints False continuously after pressing a key to minimize the window.

[tsadama]

I tried the above code and it looks like it’s working! To test the code without manually pressing a key, I used pygame.event to post a keydown event. I will make a commit once I factor this in to the unit test and make sure everything’s working locally

[tsadama]

Update: the previous commits were my attempts to fix get_active. However, the CI builds are all terminating because of a lack of output for 10 + minutes. I thought this might have been because of an infinite loop, but when I commented out the loop, the CI builds are still terminating.

[robertpfeiffer]

I would expect this test to fail if the user is using a tiling window manager.

[MyreMylar]

OK, I investigated this a bit.

1. From what I can tell you can trim the failing part of the test right down to something like this:

pygame.init()

pygame.display.set_mode((640, 480))

pygame.event.clear()
pygame.display.iconify()

self.assertEqual(pygame.display.get_active(), False)

On windows and so long as you have also added the C code change above it will pass locally.

2. This part of the test at least will not work on the CI (Travis & Appveyor) as iconify() doesn't work with the 'dummy' video driver. This means you need to split the test into two parts and add a skip decorator to the top. that looks something like this:

    @unittest.skipIf(
        os.environ.get("SDL_VIDEODRIVER") == "dummy",
        'requires the SDL_VIDEODRIVER to be a non "dummy" value',
    )
    def test_get_active_iconify(self):

3. I'm not sure what the issue is on Ubuntu but if the original test code was working for you and the test above doesn't then it sounds like it might be related to calling display.update() and event.pump() a few times so I would be tempted to just a a for loop that calls those functions a few times after calling iconify and seeing if that resolves the difference. I'll have a look on raspberry pi later myself.

[MyreMylar]

Requesting changes as indicated above.

[tsadama]

Hi I made the changes requested above, but I am still getting the issue where my CI builds are terminating because of a lack of output for 10 minutes. The previous 3 commits were to make sure everything is updated with my fork/branch, but I don't know where the issue stems from.

[MyreMylar]

From what I can tell Travis is failing on the SDL1 build on line 74. It looks to me it like there is an arguable bug in SDL 1 there. It appears that:

return PyBool_FromLong((SDL_GetAppState() & SDL_APPACTIVE) != 0);

is returning True before a window has even been created.

I think we could make the case for changing SDL 1's get_active to something like:

#else  /* IS_SDLv1 */
static PyObject *
pg_get_active(PyObject *self, PyObject *args)
{
    if (!pgDisplaySurfaceObject)
         return PyBool_FromLong(0);
    return PyBool_FromLong((SDL_GetAppState() & SDL_APPACTIVE) != 0);
}
#endif /* IS_SDLv1 */

to fit better with how the docs describe the function working only after set_mode has been called. That's not tested code, that's just me quickly eyeballing it to guess what I think might work.

Both of these new tests passed on travis on SDL2 so if there is something else up with the Travis builds it is almost certainly not your fault. It looks like a whole bunch of new pull requests were merged so perhaps one of them has sewed chaos.

Appveyor hasn't caught up with this PR yet either, but if it passes on there it is probably just Travis having a bad day which happens occasionally. I would try to fix the SDL1 problem either by changing the display.c code (better) or skipping that part of the test on SDL1 (OK). You should be able to test on SDL1 locally by changing the build config and then doing a clean and rebuild, something like:

python -m buildconfig -sdl1
python setup.py clean --all
python setup.py install
pip install . -U 

Or however you do that process on your platform/build environment. You can switch back to sdl2 by doing the same thing but with sdl2 in place of sdl1 on the first line.

[MyreMylar]

I think probably the toggle fullscreen problems are why this is causing the CI to lock up.

[MyreMylar]

I expect there may still be some edge cases where the toggle_fullscreen() test fails but given it only runs locally and most of us don't personally have the many potential configurations & platforms that pygame could run under I think it might be easier to see if we get any reports of failures for this test and then figure out how to skip the test in that particular situation.

REMINDER: Raise issue to update toggle_fullscreen() docs for pygame 2. It definitely runs on more platforms than just X11 now.

[MyreMylar]

Checked the current commit on windows under SDL1 & SDL2 and all the tests pass (with the windows get_active() change for SDL2).

I'll submit the change to SDL2's version of get_active() (the one from above that makes it work on windows) as a separate PR, unless you want to roll it into here?

[MyreMylar]

LGTM 👍

[tsadama]

I don't mind if you submit the SDL2 changes as a separate PR! Thank you for all the help @MyreMylar !

[MyreMylar]

Added issue about updating the toggle_fullscreen() docs here: #1990

[MyreMylar]

Just holding off merging this until the windows fix can get merged at the same time.