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

display.get_driver() test and fix #1911

Merged
merged 3 commits into from Jun 5, 2020
Merged

Conversation

gabsmoreira
Copy link
Contributor

Add unit test: display.get_driver() and closes #1744

Copy link
Contributor

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Two things:

  1. I suspect the list of SDL video drivers is out of date. I believe the SDL wiki & the pygame docs are not keeping pace with the current state of the SDL codebase, the SDL wiki only claims to have a 'partial' list.
    For example, I suspect there is a driver called 'wayland' on linux that is becoming fairly popular. The folders visible here are probably a decent guide to the video drivers that actually exist right now:

https://github.com/spurious/SDL-mirror/tree/6b6170caf69b4189c9a9d14fca96e97f09bbcc41/src/video

though those will be for SDL 2 and I suppose their driver names might not correspond exactly with the folder names, so you may profit best from a set union with your current list and those ones above for a better chance to catch-them-all.

  1. You should perhaps try calling pygame.display.quit() and seeing if a call to get_driver() then returns None. It looks like the code does something like that.

@gabsmoreira
Copy link
Contributor Author

Thanks for your quick reply @MyreMylar, I added more drivers and tested get_driver() after display.quit() as requested.

@MyreMylar
Copy link
Contributor

MyreMylar commented Jun 4, 2020

The CI tests on Travis are failing on that one specific build because that is our SDL 1 build and this function has a slightly different definition for SDL 1 and SDL 2.

Right now on SDL 1 the function calls the VIDEO_INIT_CHECK() macro before doing anything:

pygame/src_c/_pygame.h

Lines 170 to 172 in c10a68e

#define VIDEO_INIT_CHECK() \
if (!SDL_WasInit(SDL_INIT_VIDEO)) \
return RAISE(pgExc_SDLError, "video system not initialized")

which as you can see raises an assert if the display is not initialised.

I think the best fix for this would likely be to add the VIDEO_INIT_CHECK() to the sdl2 version of this function here (after line 701):

pygame/src_c/display.c

Lines 697 to 706 in 9e8fedc

#if IS_SDLv2
static PyObject *
pg_get_driver(PyObject *self, PyObject *args)
{
const char *name = NULL;
name = SDL_GetCurrentVideoDriver();
if (!name)
Py_RETURN_NONE;
return Text_FromUTF8(name);
}

In basically the same place as it is in the sdl1 version seen here:

pygame/src_c/display.c

Lines 741 to 749 in 9e8fedc

static PyObject *
pg_get_driver(PyObject *self, PyObject *args)
{
char buf[256];
VIDEO_INIT_CHECK();
if (!SDL_VideoDriverName(buf, sizeof(buf)))
Py_RETURN_NONE;
return Text_FromUTF8(buf);
}

And then change the test to check this assert is being being raised using something like:

display.quit()
with self.assertRaises(errorTypeHere):
     driver = display.get_driver()

That should make the function's behaviour consistent on both SDL 1 and SDL 2.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Thank you :)
It looks like @MyreMylar's requests were addressed, so I'll merge it.

@illume illume merged commit c6e9a03 into pygame:master Jun 5, 2020
Copy link
Contributor

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

👍

@gabsmoreira gabsmoreira deleted the issue-#1744 branch June 5, 2020 12:52
@illume illume added the display pygame.display label Sep 12, 2020
@illume illume changed the title Add unit test: display.get_driver() display.get_driver() test and fix Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display pygame.display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit test: display.get_driver()
3 participants