<html>
<head>
<base href="https://bugzilla.gnome.org/" />
</head>
<body><span class="vcard"><a href="page.cgi?id=describeuser.html&login=otaylor%40redhat.com" title="Owen Taylor <otaylor@redhat.com>"> <span class="fn">Owen Taylor</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Wrong (ultra tiny/small) cursor size on HiDPI screen"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744932">bug 744932</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #309538 status</td>
<td>none
</td>
<td>needs-work
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Wrong (ultra tiny/small) cursor size on HiDPI screen"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744932#c111">Comment # 111</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Wrong (ultra tiny/small) cursor size on HiDPI screen"
href="https://bugzilla.gnome.org/show_bug.cgi?id=744932">bug 744932</a>
from <span class="vcard"><a href="page.cgi?id=describeuser.html&login=otaylor%40redhat.com" title="Owen Taylor <otaylor@redhat.com>"> <span class="fn">Owen Taylor</span></a>
</span></b>
<pre>Review of <span class=""><a href="attachment.cgi?id=309538&action=diff" name="attach_309538" title="Support scaling of cursor sprites given what output they are on">attachment 309538</a> <a href="attachment.cgi?id=309538&action=edit" title="Support scaling of cursor sprites given what output they are on">[details]</a></span> <a href='review?bug=744932&attachment=309538'>[review]</a>:
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.</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>