Add mouse.set_cursor unit test

[Reminisque]

This pull request is to add a unit test for mouse.set_cursor and addresses issue #1763.

I think I got all of the test cases except for when there is no memory left to allocate either bit mask.
The code below is what I need an assert for, but I don't know how to test for a lack of memory since the amount RAM is different depending on the system running and I don't know of any methods to help me.

Does anyone have any ideas?

if ((NULL == xordata) || (NULL == anddata)) {
        free(xordata);
        free(anddata);
        return PyErr_NoMemory();
    }

Also, for clarification, the documentation for mouse.set_cursor says that the width for the size of the cursor has to be a multiple of 8 but not for height. Is it safe to assume that the height does not need to be a multiple of 8?

If there's anything I need to change or add, please feel free to let me know. Thank you.

[MightyJosip]

First of all thank you for your first contribution 🎉. You wrote really good tests that covers everything that should be covered. About MemoryError I don't think we check that anywhere (and it is not really that much important, nor easy to be tested). About height it is true it doesn't need to be the multiple of 8 (but you already check that here) so you don't need to edit anything more. For now I won't merge this yet, someone else may have more thoughts on this PR, but you did great job.

[Reminisque]

@MightyJosip Thank you for checking my code and for the clarification.

[Reminisque]

Sorry for accidentally closing this pull request.

I had made a mistake on a commit, pushed it and wanted to undo it, but the instructions I followed reset the branch to before any commits and I ended up force pushing an empty branch. I redid the commits so the first commit will be the same as the original first commit and the second commit has the changes I actually wanted.

[MyreMylar]

I'm actually a bit surprised that this bit:

pygame.mouse.set_cursor(size, hotspot, xormask, andmask)
self.assertEqual(pygame.mouse.get_cursor(), expected_cursor)
pygame.mouse.set_cursor(size, hotspot, list(xormask), list(andmask))
self.assertEqual(pygame.mouse.get_cursor(), expected_cursor)    

seems to run on SDL2 CI given that the last part of the test uses pygame.mouse.get_cursor() and the docs say:

Note This method is unavailable with SDL2, as SDL2 does not provide the underlying code to implement this method.

and the SDL2 get cursor code looks like this:

mouse_get_cursor(PyObject *self)
{
    return RAISE(PyExc_TypeError, "The get_cursor method is unavailable in SDL2");
}

Probably should figure out what is happening in there before approving this. Otherwise it looks pretty good :)

As a related side note for anyone else reading this; I notice that SDL2 now lets you directly set a surface as a mouse pointer - might be a good extension to the mouse module rather than having surfaces following the mouse pointer position as is common now.

See:
https://wiki.libsdl.org/SDL_CreateColorCursor

[Reminisque]

Is there a way to get the cursor values without the use of mouse.get_cursor in SDL2?

[MyreMylar]

Is there a way to get the cursor values without the use of mouse.get_cursor in SDL2?

Not as far as I can tell. SDL 2's SDL_Cursor struct just looks like this:

struct SDL_Cursor
{
    struct SDL_Cursor *next;
    void *driverdata;
};

https://github.com/spurious/SDL-mirror/blob/6b6170caf69b4189c9a9d14fca96e97f09bbcc41/src/events/SDL_mouse_c.h#L30-L34

Whereas in SDL 1 it clearly has more data in it, as you can see from the pygame SDL1 get_cursor code here:

pygame/src_c/mouse.c

Lines 299 to 308 in 7d3900d

	cursor = SDL_GetCursor();
	if (!cursor)
	return RAISE(pgExc_SDLError, SDL_GetError());
	w = cursor->area.w;
	h = cursor->area.h;
	spotx = cursor->hot_x;
	spoty = cursor->hot_y;
	size = cursor->area.w * cursor->area.h / 8;

It looks like SDL2 hands off cursor data to each platform and lets the platforms decide what data to store in the driverdata void pointer. You can see an example of this here in the cocoamouse implementation:

static SDL_Cursor *
Cocoa_CreateDefaultCursor()
{ @autoreleasepool
{
    NSCursor *nscursor;
    SDL_Cursor *cursor = NULL;

    nscursor = [NSCursor arrowCursor];

    if (nscursor) {
        cursor = SDL_calloc(1, sizeof(*cursor));
        if (cursor) {
            cursor->driverdata = nscursor;
            [nscursor retain];
        }
    }

    return cursor;
}}

https://github.com/spurious/SDL-mirror/blob/6b6170caf69b4189c9a9d14fca96e97f09bbcc41/src/video/cocoa/SDL_cocoamouse.m#L68-L86

I think you''l just have to check that set_cursor doesn't raise an SDL error on SDL2 and leave it at that. I'm not sure it's even possible to reliably screenshot the mouse pointer on all platforms to check it in that convoluted fashion, and if you could it wouldn't work on the CI anyway with the dummy video driver.

I would just make an if SDL1 branch that uses your current test formula and and SDL2 else branch that just checks that the same calls to set cursor don't raise any SDL errors from this RAISE here:

pygame/src_c/mouse.c

Lines 266 to 267 in 7d3900d

	if (!cursor)
	return RAISE(pgExc_SDLError, SDL_GetError());

[Reminisque]

Would this be sufficient in the SDL2 branch?

try:
    pygame.mouse.set_curor(size, hotspot, xormask, andmask)
except pygame.error:
    with self.assertRaises(pygame.error):
        pygame.mouse.set_cursor(size, hotspot, xormask, andmask)

[MyreMylar]

Would this be sufficient in the SDL2 branch?

try:
    pygame.mouse.set_curor(size, hotspot, xormask, andmask)
except pygame.error:
    with self.assertRaises(pygame.error):
        pygame.mouse.set_cursor(size, hotspot, xormask, andmask)

I think you can probably simplify it down, something like:

if SDL1:
  # current test goes here
  ...
else:
    # get_cursor doesn't work under SDL2, so we just check that set_cursor() doesn't raise an error when called
    # with data we expect to work.
    try:
         pygame.mouse.set_cursor(size, hotspot, xormask, andmask)
    except pygame.error:
        self.fail()

Hopefully that makes sense. We want the test to fail if set_cursor() raises an unexpected exception on SDL2 (i.e. we think our passed parameters are good but SDL is raising an error). I'm not aware of it being supposed to fail on any platform - but if we find one later we can skip the test on that platform then.

[Reminisque]

The CI checks are currently failing when there is no cursor present for SDL2 since it's being caught in the except block and self.fail() is subsequently called.

FAIL: test_set_cursor (pygame.tests.mouse_test.MouseModuleTest)
Ensures set_cursor works correctly.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.8.0/lib/python3.8/site-packages/pygame-2.0.0.dev11-py3.8-linux-x86_64.egg/pygame/tests/mouse_test.py", line 110, in test_set_cursor
    pygame.mouse.set_cursor(size, hotspot, xormask, andmask)
pygame.error: Cursors are not currently supported
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.8.0/lib/python3.8/site-packages/pygame-2.0.0.dev11-py3.8-linux-x86_64.egg/pygame/tests/mouse_test.py", line 112, in test_set_cursor
    self.fail()
AssertionError: None

[MyreMylar]

Looking into this it looks like set_cursor() works fine running locally, but fails out on SDL2 with the dummy video driver. It literally fails in the SDL2 code here:

    if (!mouse->CreateCursor) {
        SDL_SetError("Cursors are not currently supported");
        return NULL;
    }

https://github.com/spurious/SDL-mirror/blob/3384f7b2845eeeae887386d0879a24d066678775/src/events/SDL_mouse.c#L946-L949

So, I think at this point it is probably a good idea to split this into two tests.

1. An SDL2 test that tests everything the SDL1 version does - apart from that the expected successful result is equal to the result of get_cursor(). This test will be skipped when SDL1 is true, or when SDL_VIDEODRIVER is dummy.

2. An SDL1 version that is the current test with the SDL2 bits removed. This test will be skipped if SDL1 is not True.

I would just call them test_set_cursor__sdl1() and test_set_cursor__sdl2().

[MyreMylar]

OK, that all looks good to me now. Runs locally and on CI under SDL1 and SDL2.

[Reminisque]

I looked over my code again and realized that expected_cursor variable is not used in SDL2, so I removed it. I also forgot to check that lists for the masks worked in SDL2, so I added an assert for that.

I changed the parameters for the calls to set_cursor in the try block to use the variables at the top which are expected to work. In the SDL1 version I had some different variables that were also in the SDL2 version. My reasoning was to use a different set of values so that in the subsequent calls to set_cursor you would know that the cursor values are actually changing. In SDL2, there isn't a way to check the cursor values after the call, so it seemed better to just use the variables I already had.

Besides the unused variable being removed, are the other changes alright?

[MyreMylar]

Yep, that all seems fine. At some point (it's an ongoing project) we'll run all the test files through linting tools like pylint which will identify/clean up things like unused variables.

[Reminisque]

Gotcha. Thanks for the help and insight from everyone.