<html>
<head>
<base href="https://bugzilla.gnome.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - wayland: Support HiDPI pointer cursors"
href="https://bugzilla.gnome.org/show_bug.cgi?id=746141#c10">Comment # 10</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - wayland: Support HiDPI pointer cursors"
href="https://bugzilla.gnome.org/show_bug.cgi?id=746141">bug 746141</a>
from <span class="vcard"><a href="page.cgi?id=describeuser.html&login=jadahl%40gmail.com" title="Jonas Ådahl <jadahl@gmail.com>"> <span class="fn">Jonas Ådahl</span></a>
</span></b>
<pre>(In reply to Matthias Clasen from <a href="show_bug.cgi?id=746141#c5">comment #5</a>)
<span class="quote">> Review of <span class=""><a href="attachment.cgi?id=299291&action=diff" name="attach_299291" title="wayland: Support scaling of theme based cursors">attachment 299291</a> <a href="attachment.cgi?id=299291&action=edit" title="wayland: Support scaling of theme based cursors">[details]</a></span> <a href='review?bug=746141&attachment=299291'>[review]</a> [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 ?</span >
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.
<span class="quote">>
> @@ +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 ?</span >
Oh, right, how did I miss that.
<span class="quote">>
> ::: 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.</span >
I suppose so. But then again, maybe accessibility potentially want to make
everything extra large?
<span class="quote">>
> @@ +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.</span >
Yes, as you pointed out, it should have been set when getting it.
<span class="quote">>
> ::: 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.</span >
Sure.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are on the CC list for the bug.</li>
</ul>
</body>
</html>