<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#c116">Comment # 116</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=otaylor%40redhat.com" title="Owen Taylor <otaylor@redhat.com>"> Owen Taylor</a> <pre>(In reply to Jonas Ådahl from <a href="show_bug.cgi?id=744932#c114">comment #114</a>) > (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] [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. That's not really what "realize" means in a GTK+/Clutter context - realize means "create the resources now that everything is set up". It would seem to me there would be more flexibility from cursor_sprite_set_wl_buffer/xcursor that stored them, and let the cursor renderer pick up the data and create further backend resources at need. But anyways, let's get this landed. > > ::: 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. What about "prepare-at" ? > > ::: 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. The naming doesn't make it obvious, but disconnect_by_func takes the function *and* data.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>