<html> <head> <base href="https://bugzilla.gnome.org/" /> </head> <body> <div> <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#c114">Comment # 114</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 <a href="page.cgi?id=describeuser.html&login=jadahl%40gmail.com" title="Jonas Ådahl <jadahl@gmail.com>"> Jonas Ådahl</a> <pre>(In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c111">comment #111</a>) > Review of <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> <a href='review?bug=744932&attachment=309538'>[review]</a> [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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>