GNOME Bugzilla – Bug 749287
GDK: W32 backend does not support cursor themes
Last modified: 2015-05-20 08:56:21 UTC
Also, it uses built-in ugly X cursors as a fallback when system cursor theme does not have a particular cursor.
Created attachment 303291 [details] [review] GDK: Add cursor theme support to W32 backend Load themed cursors from the same places they are loaded on freedesktop systems, but use W32 API functions to do so (works for .cur/.ani cursors instead of X cursors). Refactor the code for cursor handling. Prefer loading cursors by name. Do not load actual cursors when loading the theme. Find the files and remember the arguments/calls for loading them instead. Keeping HCURSOR instance in the hashmap would result in multiple GdkCursors using the same HCURSOR. Given that we use DestroyCursor() to off them, this would cause problems (at the very least - DestroyCursor() would fail). Store GdkCursor instances in a cache. Update cached cursors when theme changes. Recognize "system" theme as a special (and default) case. When it is set, prefer system cursors and fall back to Adwaita cursors and (as a last resort) built-in X cursors. Otherwise prefer theme cursors and fall back to system and X cursors. Force GTK to use "left_ptr" cursor when no cursor is set. Using NULL makes it use the system default "arrow", which is not the intended behaviour when a non-system theme is selected. Ignore cursor size setting and query the OS for the required cursor size, as Windows (almost) does not allow setting cursors of arbitrary size.
Review of attachment 303291 [details] [review]: So this expects PREFIX/icons/Adwaita/cursors/ to be full of .ani/.cur cursors ? I assume recent commits in adwaita-icon-theme are in preparation for making that happen ? It would be good to document this somewhere, such as docs/reference/gtk/windows.sgml or README.win32 ::: gdk/win32/gdkcursor-win32.c @@ +173,3 @@ + struct win32_cursor *cursor; + + fullname = g_strdup_printf ("%s/%s", dir, filename); I tend to use g_strconcat to avoid gratitious use of printf @@ +197,3 @@ + cursor_name = g_strdup (filename); + + cursor = win32_cursor_new (GDK_WIN32_CURSOR_LOAD_FROM_FILE, u16_filename, size, size, LR_LOADFROMFILE | (size == 0 ? LR_DEFAULTSIZE : 0), 0, 0); A little irritating that win32_cursor_new takes ownership of u16_filename. It would be clearer if it made a copy. @@ +225,3 @@ + win32_cursor_theme_load_from (theme, size, theme_dir); + g_free (theme_dir); + } These two loops look identical to me - I guess one can go
(In reply to Matthias Clasen from comment #2) > Review of attachment 303291 [details] [review] [review]: > > So this expects PREFIX/icons/Adwaita/cursors/ to be full of .ani/.cur > cursors ? Yes. Although technically they are not *required* to have these extensions, i thought that leaving them extensionless would be just too cruel towards less savvy users, as Explorer won't be able to recognize these files as cursors (and draw thumbnails) without extensions. > I assume recent commits in adwaita-icon-theme are in preparation for making > that happen ? Yes, laying up some finishing touches today. > It would be good to document this somewhere, such as > docs/reference/gtk/windows.sgml or README.win32 Will do. Separate patch? In this bug? > @@ +225,3 @@ > + win32_cursor_theme_load_from (theme, size, theme_dir); > + g_free (theme_dir); > + } > > These two loops look identical to me - I guess one can go My bad, a copy&paste error. One was meant to say "pixels", the other - "icons".
(In reply to Matthias Clasen from comment #2) > Review of attachment 303291 [details] [review] [review]: > > @@ +197,3 @@ > + cursor_name = g_strdup (filename); > + > + cursor = win32_cursor_new (GDK_WIN32_CURSOR_LOAD_FROM_FILE, > u16_filename, size, size, LR_LOADFROMFILE | (size == 0 ? LR_DEFAULTSIZE : > 0), 0, 0); > > A little irritating that win32_cursor_new takes ownership of u16_filename. > It would be clearer if it made a copy. Possible, but there's a caveat: resource_name might be a string (wchar_t*) or it might be a resource ID (an integer disguised as wchar_t*). It's possible to tell them apart (you check the high-order word of the pointer; if it's zero, it's not a real string; also, i can check load_type and assume that GDK_WIN32_CURSOR_LOAD_FROM_FILE means string, other types mean integer), but it's a bother. Also, glib doesn't have a function for strupping a wchar_t* string.
Created attachment 303394 [details] [review] GDK: Add cursor theme support to W32 backend v2: * Add a bit of documentation * Use g_strconcat() instead of g_strdup_printf () * Fix a copy&paste typo (search "pixmaps" and "icons" instead of searching "icons" twice)
Review of attachment 303394 [details] [review]: ::: docs/reference/gtk/windows.sgml @@ +122,3 @@ +<para> +Theme can be changed at runtime using GTK Inspector, or by setting <literal>gtk-cursor-theme-name</literal> in <filename>settings.ini</filename>. +</para> Thanks for adding this documentation. I would reword this paragraph slightly, by putting the emphasis on the GTK+ setting. Maybe something like this: The cursor theme can be set with the <literal>gtk-cursor-theme-name</literal> GTK+ setting. GTK+ settings can be overridden per-user in the <filename>settings.ini</filename> file or changed at runtime in the GTK+ Inspector. ::: gdk/win32/gdkcursor-win32.c @@ +217,3 @@ + win32_cursor_theme_load_from (theme, size, theme_dir); + g_free (theme_dir); + } I'm not convinced this is useful, tbh. We don't have cursors in /usr/share/pixmaps/cursor/ on Linux. And we would really like to get away from looking for icons in /usr/share/pixmaps, in general - thats an ancient X-ism that unforunately got grandfathered into the icon theme spec. So, if you want my advice: drop the pixmaps/ location.
*** Bug 749177 has been marked as a duplicate of this bug. ***
Created attachment 303475 [details] [review] GDK: Add cursor theme support to W32 backend v3: * Reworded the docs * Removed pixmaps-searching code
Review of attachment 303475 [details] [review]: See the initial nitpicks ::: gdk/win32/gdkcursor-win32.c @@ +34,2 @@ static HCURSOR +hcursor_from_x_cursor (gint i, GdkCursorType cursor_type) split params @@ +97,3 @@ +static HCURSOR +win32_cursor_create_hcursor (struct win32_cursor *cursor) can we typedef this struct? @@ +104,3 @@ + { + case GDK_WIN32_CURSOR_LOAD_FROM_FILE: + result = LoadImageW (NULL, cursor->resource_name, IMAGE_CURSOR, cursor->width, cursor->height, cursor->load_flags); split params for these LoadImageWs @@ +123,3 @@ + +static struct win32_cursor * +win32_cursor_new (GdkWin32CursorLoadType load_type, gpointer resource_name, gint width, gint height, guint load_flags, gint xcursor_number, GdkCursorType cursor_type) split params @@ +168,3 @@ + { + gchar *fullname; + gunichar2 *u16_filename; for consistency with other parts of the code, call this filenamew ? @@ +197,3 @@ + cursor_name = g_strdup (filename); + + cursor = win32_cursor_new (GDK_WIN32_CURSOR_LOAD_FROM_FILE, u16_filename, size, size, LR_LOADFROMFILE | (size == 0 ? LR_DEFAULTSIZE : 0), 0, 0); split this a bit @@ +203,3 @@ + +static void +win32_cursor_theme_load_from_dirs (struct win32_cursor_theme *theme, const gchar *name, gint size) split params @@ +226,3 @@ + +static void +win32_cursor_theme_load_system (struct win32_cursor_theme *theme, gint size) split params @@ +240,3 @@ + /* Prefer W32 cursors */ + if (cursors[i].builtin) + hcursor = LoadImageA (NULL, cursors[i].builtin, IMAGE_CURSOR, size, size, LR_SHARED | (size == 0 ? LR_DEFAULTSIZE : 0)); split @@ +250,3 @@ + + DestroyCursor (hcursor); + cursor = win32_cursor_new (GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL, (gpointer) cursors[i].builtin, size, size, LR_SHARED | (size == 0 ? LR_DEFAULTSIZE : 0), 0, 0); split @@ +256,3 @@ + +struct win32_cursor_theme * +win32_cursor_theme_load (const gchar *name, gint size) split params @@ +258,3 @@ +win32_cursor_theme_load (const gchar *name, gint size) +{ + struct win32_cursor_theme *result = g_new (struct win32_cursor_theme, 1); can this struct be typedefed? @@ +260,3 @@ + struct win32_cursor_theme *result = g_new (struct win32_cursor_theme, 1); + + result->named_cursors = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, win32_cursor_destroy); split @@ +287,3 @@ + +struct win32_cursor * +win32_cursor_theme_get_cursor (struct win32_cursor_theme *theme, const gchar *name) split params @@ +308,3 @@ + /* Use real Win32 cursor if possible */ + if (cursors[i].builtin) + return LoadImageA (NULL, cursors[i].builtin, IMAGE_CURSOR, 0, 0, LR_SHARED | LR_DEFAULTSIZE); split this @@ +337,3 @@ } +static struct { I'd prefer to have this split in declaration and then the static struct @@ +375,3 @@ + int i; + + for (i = 0; i < G_N_ELEMENTS(default_cursors); i++) space before ( @@ +377,3 @@ + for (i = 0; i < G_N_ELEMENTS(default_cursors); i++) + { + if (0 != strcmp(default_cursors[i].name, name)) space before ( @@ +380,3 @@ + continue; + + return LoadImageA (NULL, default_cursors[i].id, IMAGE_CURSOR, 0, 0, LR_SHARED | LR_DEFAULTSIZE); split this @@ +392,3 @@ + + for (i = 0; i < G_N_ELEMENTS (cursors); i++) + if (cursors[i].name == NULL || 0 == strcmp (cursors[i].name, name)) please no yoda programming, do strcmp () == 0 @@ +484,3 @@ + if (theme_cursor != NULL) + hcursor = win32_cursor_create_hcursor (theme_cursor); + /* why this comment? @@ +529,3 @@ +_gdk_win32_display_init_cursors (GdkWin32Display *display) +{ + display->cursor_cache = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); split this @@ +595,3 @@ + } + + g_hash_table_insert (win32_display->cursor_cache, cursor_name, g_object_ref (result)); split this @@ +629,3 @@ + return result; + + g_hash_table_insert (win32_display->cursor_cache, g_strdup (name), g_object_ref (result)); split this ::: gdk/win32/gdkdisplay-win32.c @@ +30,3 @@ + * gdk_win32_display_set_cursor_theme: + * @display: (type GdkWin32Display): a #GdkDisplay + * @theme: the name of the cursor theme to use, or %NULL to unset missing (allow none) ? @@ +83,3 @@ + if (theme == NULL) + { + g_warning ("Failed to load cursor theme %s\n", name); no need for \n in g_warnings @@ +94,3 @@ + + win32_display->cursor_theme = theme; + if (win32_display->cursor_theme_name != NULL) no need for this check @@ +116,3 @@ + if (theme == NULL) + { + g_warning ("Failed to load cursor theme %s\n", no \n ::: gdk/win32/gdkprivate-win32.h @@ +382,3 @@ +void _gdk_win32_display_update_cursors (GdkWin32Display *display); + +struct win32_cursor_theme { add typedef and change the name to CamelCase @@ +393,3 @@ +} GdkWin32CursorLoadType; + +struct win32_cursor { add typedef and change to CameCase
Created attachment 303526 [details] [review] GDK: Add cursor theme support to W32 backend v4: * Fix most nitpicks * Better system theme (specifically - use IDC cursors for CSS cursors and left_ptr_watch)
Review of attachment 303526 [details] [review]: Looks good to me now, thanks
Review of attachment 303526 [details] [review]: Still some nitpicks from my side. ::: gdk/win32/gdkcursor-win32.c @@ +32,3 @@ #include "xcursors.h" +typedef struct _default_cursor { DefaultCursor maybe? @@ +203,3 @@ + Win32Cursor *cursor = data; + + if (cursor->load_type == GDK_WIN32_CURSOR_LOAD_FROM_FILE) why do you need to check this? if resource_name is NULL nothing will be done @@ +251,3 @@ + + if (dot) + cursor_name = g_strndup (filename, dot - filename); I'd probably go with a triple here @@ +362,3 @@ + gint size) +{ + Win32CursorTheme *result = g_new (Win32CursorTheme, 1); maybe g_new0 so you ensure everything is inited to 0? @@ +453,3 @@ + for (i = 0; i < G_N_ELEMENTS (default_cursors); i++) + { + if (0 != strcmp (default_cursors[i].name, name)) no yoda programming @@ +534,3 @@ + + if (name != NULL) + private->name = g_strdup (name); just do: private->name = g_strdup (name) no? if name is NULL it will be set to NULL ::: gdk/win32/gdkdisplay-win32.c @@ +56,3 @@ + gint w, h; + Win32CursorTheme *theme; + GdkWin32Display *win32_display = GDK_WIN32_DISPLAY(display); space before ( @@ +71,3 @@ + */ + if (w == h) + cursor_size = w; triple here?
(In reply to Ignacio Casal Quinteiro (nacho) from comment #13) > Review of attachment 303526 [details] [review] [review]: > > Still some nitpicks from my side. > > ::: gdk/win32/gdkcursor-win32.c > @@ +203,3 @@ > + Win32Cursor *cursor = data; > + > + if (cursor->load_type == GDK_WIN32_CURSOR_LOAD_FROM_FILE) > > why do you need to check this? if resource_name is NULL nothing will be done Because it may be non-NULL, but not a g_free()'able pointer, but an integer disguised as a pointer. cursor->load_type == GDK_WIN32_CURSOR_LOAD_FROM_FILE is clear and obvious way of checking for that (the less obvious way is (cursor->resource_name & 0xFFFF0000) > 0).
Review of attachment 303526 [details] [review]: ::: gdk/win32/gdkcursor-win32.c @@ -107,0 +134,8 @@ +static HCURSOR +win32_cursor_create_hcursor (Win32Cursor *cursor) +{ ... 5 more ... For the cases where we do the LoadImageW()/LoadImageA() thing, we could do this for the cursor->resoure_name when you do the call for LoadImageW(): IS_INTRESOURCE (cursor->resoure_name) ? MAKEINTRESOURCEW (cursor->resource_name) : cursor->resource_name; And so we can probably combine the cases for GDK_WIN32_CURSOR_LOAD_FROM_FILE, GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL, GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL, by also taking the hmodule argument into account as well. I think it would be better if we use the W variants of the Windows API for new items as far as possible though.
Hi LRN, This means, the patch itself has no MSVC incompatibilities, just to let you know. With blessings, thank you!
(In reply to Fan, Chun-wei from comment #15) > Review of attachment 303526 [details] [review] [review]: > > ::: gdk/win32/gdkcursor-win32.c > @@ -107,0 +134,8 @@ > +static HCURSOR > +win32_cursor_create_hcursor (Win32Cursor *cursor) > +{ > ... 5 more ... > > For the cases where we do the LoadImageW()/LoadImageA() thing, we could do > this for the cursor->resoure_name when you do the call for LoadImageW(): > > IS_INTRESOURCE (cursor->resoure_name) ? MAKEINTRESOURCEW > (cursor->resource_name) : cursor->resource_name; So what you mean is that the code should check that resource name is, in fact, an integer in form of a pointer. If it is, make it an integer in form of a pointer. Again. If it isn't, use it as-is. That doesn't make much sense to me at the moment. > > And so we can probably combine the cases for > GDK_WIN32_CURSOR_LOAD_FROM_FILE, GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL, > GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL, by also taking the hmodule > argument into account as well. I kind of thought of that, but ended up having distinct "GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_THIS" and "GDK_WIN32_CURSOR_LOAD_FROM_RESOURCE_NULL", because the code uses only these two, and because i did not want to have to reason about _gdk_app_hmodule staying or not staying unchanged between the call to _cursor_new and creation of the actual cursor. > I think it would be better if we use the W > variants of the Windows API for new items as far as possible though. It's mostly of no consequence for the cases where i'm using A variants, as strings are either NULL or not actually strings. Do they give you a pointer type mismatch (wchar_t* in resource_name vs char* expected by LoadImageA)?
(In reply to Fan, Chun-wei from comment #16) > Hi LRN, > > This means, the patch itself has no MSVC incompatibilities, just to let you > know. Okay, in that case i'll push it as-is, and if any problems arise, we'll burn that bridge after we cross it.
Attachment 303526 [details] pushed as 26c2432 - GDK: Add cursor theme support to W32 backend