[Wayland-bugs] [Bug 744932] Wrong (ultra tiny/small) cursor size on HiDPI screen

[mutter (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=744932

--- Comment #114 from Jonas Ådahl <jadahl at gmail.com> ---
(In reply to Owen Taylor from comment #111)
> Review of attachment 309538 [details] [review]:
> 
> I have this feeling that it somehow could be simpler (like why does
> realization have to be triggered throughout the code - couldn't it just be
> done by the backend when needed?) but as far as I can follow the code it
> seems like it should work. Some more style comments follow:

This is because the data backing the cursor may not part of the actual cursor
(the wl_surface) so if we'd want to realize it just before drawing, we wouldn't
know how to realize it, since the cursor doesn't know about the wl_surface. We
could probably add a signal instead that the creator needs to set, or a
"backing object" so that the renderer can call "cursor->backing->realize()" or
something. With the current approach the renderer specific realization (which
is what realize does) is done at creation when the source of the data is known.

> 
> ::: src/backends/meta-cursor.c
> @@ +350,3 @@
>    object_class->finalize = meta_cursor_sprite_finalize;
> +
> +  signals[PRE_PAINT_AT] = g_signal_new ("pre-paint-at",
> 
> I dislike this naming - the other places where we use "pre-paint", we use it
> specifically for things done in the clutter PRE_PAINT phase.
> "position-updated" would be better.

"position-updated" is not really correct though (I realize the calling function
name makes it look like it), but it will be signalled even if the position
didn't change, but the cursor changed. Maybe just "cursor-updated" or
something.

> 
> ::: src/wayland/meta-wayland-pointer.c
> @@ +809,3 @@
> +  prev_cursor_surface = pointer->cursor_surface;
> +  if (prev_cursor_surface)
> +    g_signal_handlers_disconnect_by_data (cursor_tracker,
> prev_cursor_surface);
> 
> Neither cursor_tracker nor prev_cursor_surface is private to this part of
> the code, so you can't be *sure* that nobody else has made a connection.
> g_signal_handlers_disconnect_by_func() should always be used unless the data
> is truly private.

Well, we can't disconnect by_func because that would make life harder if we one
day want to support multiple seats.

> 
> Is there any reason not to make the connection *once*, and simply update
> pointer->cursor_surface, rather than connecting like this?

I suppose that might work. And can just use a handler id to disconnect to avoid
any trouble with disconnecting.

> 
> @@ +848,3 @@
> +        {
> +          cursor_role->cursor_sprite = meta_cursor_sprite_new ();
> +          g_signal_connect_object (cursor_role->cursor_sprite,
> 
> I'd honestly just disconnect in the dispose function for the cursor role -
> it's a lot more straightforward. With this, there is going to be some
> interval during destruction of the cursor sprite where the signal is
> disconnected but cursor_role->cursor sprite is set, or vice versa, and that
> can lead to head-banging hard bugs. Maybe not here, but I'd consider this a
> pattern to avoid

Indeed. I already do this locally, because there was an occasional crash where
the listener assumed there was a valid surface->role pointer.

> 
> @@ +942,3 @@
>  
> +static void
> +cursor_surface_dispose (GObject *object)
> 
> surface_role_cursor_dispose() [or
> meta_wayland_surface_role_cursor_dispose()] it should be clear what object
> it's a dispose function for.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-bugs/attachments/20150904/3a9c305f/attachment-0001.html>