Re: [Patch] Cursor cache

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

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.

+  cursor_cache = g_slist_prepend(cursor_cache, cursor);

We put a space between the function and the opening (

+  GdkCursorPrivate* curs=(GdkCursorPrivate*)listelem;

and around = in assignments

+  if ((curs->cursor.type!=key->type) ||

and around binary operators like !=, >, ==


+  if (display->closed) {
+  } else {

Parens go on a new line

+    if (private)
+    {

and are indented

+	{
+		XFixesChangeCursor (xdisplay, new_cursor, private->xcursor);
+		private->xcursor=new_cursor;
+	}

basic indent is 2, not 8


* We need to purge cursors from the cache when their display is closed
>
> OK - is there a hook for that? Also is there an easy way to test
> multidisplay code like that - what can I easily get to open something
> on two displays?

gtk-demo has a 'change display'  demo.
gdk/x11/gdkdisplay-x11.c:gdk_display_x11_finalize is the right place, I think.


>> * Do you have any data how big the cache grows in cursor-heavy apps ?
>
> No, but I've not found anything that uses more than 3 or 4 cursors;
> they just open the same damn ones over and over again.  The overhead
> for each cache entry is small, cursors seem to vary on disc, but seem
> to be between 15 and 35k except for some big ones but you are only paying
> a penalty for those that are used rarely.

Ah, I was really interested in the length of the list. That sounds fine then.


>> Wrt. to blank cursors, adding a new cursor type or special-casing a
>> cursor name like "none" would work, although we probably cannot change
>> GDK_LAST_CURSOR, judging from google code search results.
>
> It's a pity if GDK_LAST_CURSOR has to stay put, so it probably doesn't
> make sense to add a new type. Special caseing the name sounds a bit
> odd; since in either case it's effectively changing the API would it
> be cleaner just to have a new function call for it?
> The other way I'd considered was special casing the pixmap code for 1x1 -
> but again that feels dirty.

GDK_BLANK_CURSOR = -2 should work fine.