Re: [Patch] Cursor cache

["Dr. David Alan Gilbert" <gilbertd treblig org>]

* Matthias Clasen (matthias clasen gmail com) wrote:
> On Sun, Jan 18, 2009 at 12:24 PM, Dr. David Alan Gilbert
> >
> > 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.

Yep, that still needs to be cleaned up in the version you checked in
I think replacing the ', cursors are added to it but never removed'
by '. Cursors are only removed from the cache when Displays are closed.'


> > 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.

I wonder how many other copies of similar code are in apps?

> > 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.

Does it get used?  If it was used then you would think there would be a way
for apps to look up to see if the XServer already has it rather than actually
search the theme?

> > 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.

Yes thanks; I see your change in the version you committed - thanks.

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

Thanks.

> 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().

Yeh I guess apps might be silly and use both in the same app.

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

One worry I had left was in the theme change code in
gdk_x11_cursor_update_theme there is no check for the blank case
so it will end up calling XcursorShapeLoadCursor with our new -2
made up type; it seems to work - I think because it only changes
it if this returns non-Null; but perhaps it's safer to add:

Index: gdk/x11/gdkcursor-x11.c
===================================================================
--- gdk/x11/gdkcursor-x11.c	(revision 22204)
+++ gdk/x11/gdkcursor-x11.c	(working copy)
@@ -575,6 +575,9 @@
 
   if (private->xcursor != None)
     {
+      if (cursor->type == GDK_BLANK_CURSOR)
+	return;
+
       if (cursor->type == GDK_CURSOR_IS_PIXMAP)
 	{
 	  if (private->name)

===================================================================

Thanks,

Dave
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    | Running GNU/Linux on Alpha,68K| Happy  \ 
\ gro.gilbert @ treblig.org | MIPS,x86,ARM,SPARC,PPC & HPPA | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/