GNOME Bugzilla – Bug 746141
wayland: Support HiDPI pointer cursors
Last modified: 2015-03-16 11:56:38 UTC
Given the outputs a pointer cursor is on, provide an appropriate buffer given the highest output scale. This patch assumes no global HiDPI configuration has been set. I don't have HiDPI hardware and I have not looked into how theme sizes and such gets calculated, but I suspect something needs to be added to counter that. Depends on https://bugzilla.gnome.org/show_bug.cgi?id=744932 to function properly.
Created attachment 299290 [details] [review] wayland: Don't set cursor surface scale when we don't have a surface The setting of the the surface scale even when the surface is not created from a surface was introduced due to a crash when getting the buffers when dividing by the scale. The only reason I can see this is that we get the buffer from a non-existing surface when the wl_cursor has not yet been set. Instead, use the name field to avoid trying to use the non-existing surface, effectively avoiding the division-by-zero that way.
Created attachment 299291 [details] [review] wayland: Support scaling of theme based cursors Support scaling of cursors created from themes. The default scale is always 1, but if the pointer cursor surface enters an output with a higher scale, load the larger version of the cursor theme and use the image from that theme. This assumes the theme size is set to one that fits with an output scale = 1.
Created attachment 299292 [details] [review] wayland: Ignore setting the same cursor theme as was already set If the name and size of the theme is identical to the current configuration, do nothing.
Created attachment 299293 [details] [review] wayland: Don't set the wl_surface user_data twice wl_surface_add_listener already sets the user data pointer, so no need to do it separately before.
Review of attachment 299291 [details] [review]: ::: gdk/wayland/gdkdisplay-wayland.c @@ +623,3 @@ + g_assert (scale >= 1); + + theme = wayland_display->scaled_cursor_themes[scale - 2]; scale - 2 ?! That seems to rely on the struct ordering to access the cursor_theme member _before_ the array if scale is 1 ? Can we just put it all in the array, and use scale - 1 here ? @@ +634,3 @@ + wayland_display->cursor_theme_name, scale); + return NULL; + } I guess this is where we would expect the theme to be stuffed in that array ? ::: gdk/wayland/gdkdisplay-wayland.h @@ +44,3 @@ +#define GDK_WAYLAND_MAX_SCALED_THEMES 5 +#define GDK_WAYLAND_MAX_THEME_SCALE (GDK_WAYLAND_MAX_SCALED_THEMES + 1) 5 seems excessive. In practice, we're dealing with scales of 1 or 2 only. @@ +75,2 @@ struct wl_cursor_theme *cursor_theme; + struct wl_cursor_theme *scaled_cursor_themes[GDK_WAYLAND_MAX_SCALED_THEMES]; Nothing ever sets the scaled_cursor_themes, afaics. ::: gdk/wayland/gdkwindow-wayland.c @@ -38,3 @@ #include <errno.h> -#define WL_SURFACE_HAS_BUFFER_SCALE 3 Moving these 'capability' defines to a single shared place is a good idea. Might be nice to do as a cleanup patch beforehand.
Review of attachment 299291 [details] [review]: ::: gdk/wayland/gdkcursor-wayland.c @@ +99,3 @@ + else + theme = _gdk_wayland_display_get_scaled_cursor_theme (wayland_display, + cursor->scale); Ah, I see, you're currently only calling that function with a scale > 1. But why not always call it, and do away with the extra complication ?
Review of attachment 299290 [details] [review]: sure, seems fine
Review of attachment 299292 [details] [review]: sure
Review of attachment 299293 [details] [review]: ok
(In reply to Matthias Clasen from comment #5) > Review of attachment 299291 [details] [review] [review]: > > ::: gdk/wayland/gdkdisplay-wayland.c > @@ +623,3 @@ > + g_assert (scale >= 1); > + > + theme = wayland_display->scaled_cursor_themes[scale - 2]; > > scale - 2 ?! > That seems to rely on the struct ordering to access the cursor_theme member > _before_ the array if scale is 1 ? > > Can we just put it all in the array, and use scale - 1 here ? As you mentioned, the "scaled_cursor_themes" are only for the scaled themes, i.e. scale 1 is stored separately. I suppose we could just put it together to avoid the confusion. > > @@ +634,3 @@ > + wayland_display->cursor_theme_name, scale); > + return NULL; > + } > > I guess this is where we would expect the theme to be stuffed in that array ? Oh, right, how did I miss that. > > ::: gdk/wayland/gdkdisplay-wayland.h > @@ +44,3 @@ > > +#define GDK_WAYLAND_MAX_SCALED_THEMES 5 > +#define GDK_WAYLAND_MAX_THEME_SCALE (GDK_WAYLAND_MAX_SCALED_THEMES + 1) > > 5 seems excessive. In practice, we're dealing with scales of 1 or 2 only. I suppose so. But then again, maybe accessibility potentially want to make everything extra large? > > @@ +75,2 @@ > struct wl_cursor_theme *cursor_theme; > + struct wl_cursor_theme > *scaled_cursor_themes[GDK_WAYLAND_MAX_SCALED_THEMES]; > > Nothing ever sets the scaled_cursor_themes, afaics. Yes, as you pointed out, it should have been set when getting it. > > ::: gdk/wayland/gdkwindow-wayland.c > @@ -38,3 @@ > #include <errno.h> > > -#define WL_SURFACE_HAS_BUFFER_SCALE 3 > > Moving these 'capability' defines to a single shared place is a good idea. > Might be nice to do as a cleanup patch beforehand. Sure.
(In reply to Jonas Ådahl from comment #10) > > > > ::: gdk/wayland/gdkdisplay-wayland.h > > @@ +44,3 @@ > > > > +#define GDK_WAYLAND_MAX_SCALED_THEMES 5 > > +#define GDK_WAYLAND_MAX_THEME_SCALE (GDK_WAYLAND_MAX_SCALED_THEMES + 1) > > > > 5 seems excessive. In practice, we're dealing with scales of 1 or 2 only. > > I suppose so. But then again, maybe accessibility potentially want to make > everything extra large? We use the (nominal) theme size for that, and set cursor_size to 24, 48 or 64. And we have a separate (shader-based) magnifier in the shell, so we don't have to abuse hi-dpi scale for accessibility.
Created attachment 299481 [details] [review] wayland: Don't set cursor surface scale when we don't have a surface The setting of the the surface scale even when the surface is not created from a surface was introduced due to a crash when getting the buffers when dividing by the scale. The only reason I can see this is that we get the buffer from a non-existing surface when the wl_cursor has not yet been set. Instead, use the name field to avoid trying to use the non-existing surface, effectively avoiding the division-by-zero that way.
Created attachment 299482 [details] [review] wayland: Put interface version defines in a common place So far only one, but put it somewhere all files can see it.
Created attachment 299483 [details] [review] wayland: Support scaling of theme based cursors Support scaling of cursors created from themes. The default scale is always 1, but if the pointer cursor surface enters an output with a higher scale, load the larger version of the cursor theme and use the image from that theme. This assumes the theme size is set to one that fits with an output scale = 1.
(In reply to Jonas Ådahl from comment #12) > Created attachment 299481 [details] [review] [review] > wayland: Don't set cursor surface scale when we don't have a surface > > The setting of the the surface scale even when the surface is not > created from a surface was introduced due to a crash when getting the > buffers when dividing by the scale. The only reason I can see this is > that we get the buffer from a non-existing surface when the wl_cursor > has not yet been set. > > Instead, use the name field to avoid trying to use the non-existing > surface, effectively avoiding the division-by-zero that way. The original version had missing fallback NULL return. The new version fixes that.
Review of attachment 299481 [details] [review]: .
Review of attachment 299482 [details] [review]: ok
Review of attachment 299483 [details] [review]: looks good to me now
Attachment 299292 [details] pushed as 7372ddd - wayland: Ignore setting the same cursor theme as was already set Attachment 299293 [details] pushed as 225c10b - wayland: Don't set the wl_surface user_data twice Attachment 299481 [details] pushed as c7be8fd - wayland: Don't set cursor surface scale when we don't have a surface Attachment 299483 [details] pushed as 465647e - wayland: Support scaling of theme based cursors Attachment 299482 [details] pushed as 7b2cdab - wayland: Put interface version defines in a common place