Re: [Patch] Cursor cache

["Matthias Clasen" <matthias clasen gmail com>]

On Sun, Jan 18, 2009 at 12:24 PM, Dr. David Alan Gilbert
<gilbertd treblig org> wrote:
> * Matthias Clasen (matthias clasen gmail com) wrote:
>> On Sat, Jan 17, 2009 at 8:19 PM, Dr. David Alan Gilbert
>> <gilbertd treblig org> wrote:
>> > * Matthias Clasen (matthias clasen gmail com) wrote:
>> >> Thanks for the patch. It looks good in general, just a couple of notes:
>> >>
>> >> * There are some stylistic issues (mostly missing spaces here and there)
>> >
>> > No problem; please point out a few of the ones that need fixing and
>> > I'll try and make they all get done.
>>
>
> OK, attached is a new patch that hopefully fixes the formatting;
> if you spot any others I'll be happy to fix them (even if I do
> feel sorry for the {'s lonely on the line by themselves)

There's still a few cases of "} else {", I think. But nothing major.

The comment above the cursor_cache definition about cursors being never
removed is not strictly true anymore.

> OK, the attached patch has untested code for that - is there an easy
> way to delete everything in an slist that matches a particular requirement?
> (a foreach where the function returned a flag to say delete?)

No, I think for an slist, the code you have there is about the
simplest that will work.
One way to make this loop easier would be to switch to a GList.

> (Also is the GDK_DISPLAY_OBJECT the right way of getting the GdkDisplay*
> pointer in gdkdisplay-x11.c ?)

Yes, thats fine.

>
> Yeh, the only one that worries me a bit is the watch animations, they seem
> to be quite big.  (It's a pity there is no way to store this data on the Xserver
> and share between all apps).

Oh, but it is. See the CreateAnimCursor request in the XRender extension.


>> GDK_BLANK_CURSOR = -2 should work fine.
>
> OK, that patch has code in there to do that - which seems to work
> (tried with a patched libvte - patch attached)
> However it needs some work.  It works out quite nicely
> because it just ends up getting cached like anything else created
> through gdk_cursor_new_for_display; my 'get_blank_cursor' function
> doesn't try to do anything smart - so the first lookup still ends
> up going through XCursor's theme code; I'll look at that again next week.

I don't think thats a problem at all, since all later accesses will
find it in the cache.

> But I've got some questions about how things should work:
>
>    1) I'm confused by GdkColor's - the code in Vte initialises black by
>       doing:
>
>       GdkColor black = {0,0,0}
>
>       but that type has 4 elements - including 'pixel'; so that initialiser
>       initialises pixel, red, and green - but not blue.  What's the right
>       way of getting a GdkColor for black?

It really doesn't matter at all in this case, since the mask makes it all
transparent anyway.  It is a bit silly to have two separate color structs with
the same content though. Compare to gtkentry.c:set_invisible_cursor.

>    2) The code seems schizophrenic as to whether we care about the
>       screen or not;
>       it looks like bitmaps should be created with a drawable for the
>       correct screen since we could have two screens with different
>       depths - but there are other places (like the code I copied)
>       that just use the default screen; and most of the code in
>       gdkcursor just takes a Display.  libXCursor seems the same, but
>       gdk_window_get_pointer has a check for getting a valid window to
>       be multihead safe.  What's the right thing to do?

The screen is irrelevant when creating pixmap cursors (all involved
pixmaps are depth 1, anyway). From the  XCreatePixmapCursor man page:

       Both source and mask, if specified, must have depth one (or a
BadMatch error
       results) but can have any root.

> Also, before GDK_BLANK_CURSOR can be used I guess the none-x11 versions
> have to be fixed up as well - I'll need a hand with that since I just
> have Linux.

Yes, we usually rely on the backend fairies to fill in gaps like that (Hi Tor!).

> As I say, this version hasn't had much testing - so use at your own risk;
> I'll fix and test it more next weekend - all comments welcome.

Looks great to me, and almost ready to go in.

One thing that occurred to me is that you can theoretically make the
cache a bit more efficient by realizing that there is overlap between
the named cursors and the font cursors. Ie the cursors returned by
creating a font cursor with type GDK_GUMBY and a named cursor with the
name "gumby" should be identical (libXcursor returns different XIDs,
but they are referring to the same object).  Xcursor seems to have a
function to match up types with names: XCursorLibraryShape().

In practise, it is probably not worth the effort, since nobody uses
named cursors anyway...