[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

Owen Taylor <otaylor at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #309538|none                        |needs-work
             status|                            |

--- Comment #111 from Owen Taylor <otaylor at redhat.com> ---
Review of attachment 309538:

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:

::: 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.

::: 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.

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

@@ +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

@@ +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/20150903/798d4636/attachment.html>