<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#c118">Comment # 118</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#c117">comment #117</a>) > I have just pushed a new version of the wip/hidpi-cursor branch to github. > Do you want me to update the patches in this bug? It would be helpful to me if you reattached the branch, and then went through and marked a-c-n anything that either a) I already marked a-c-n and you haven't substantially changed b) anything where I asked for simple changes, and you went ahead and made them and don't feel a need for re-review. > In summary, it fixes most of the issues you raised, but some things I will > comment on more explicitly: > > I did not change nor rename any of the role-ness of XWayland wl_surfaces. I > believe making it a role internally still makes most sense. OK, since you and Jasper seem to be in agreement that this makes sense, I"ll withdraw my objection. > (In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c110">comment #110</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]: > > > > ::: src/backends/meta-cursor-renderer.c > > @@ +65,2 @@ > > if (priv->displayed_cursor && !priv->handled_by_backend) > > + texture = meta_cursor_sprite_get_cogl_texture (priv->displayed_cursor); > > > > Shouldn't this use cursor_sprite? - or alternatively, why is cursor_sprite > > passed in when it seems like it will always be priv->displayed_cursor? > > I still avoid using renderer_priv->displayed_cursor or > meta_cursor_renderer_get_cursor() because my longer-term intention is to get > rid of that state, considering that it is just a duplication of the same > state tracked by meta-cursor-tracker.c. I avoid it even further in the newer > version, but not completely. > > > ::: src/backends/meta-cursor-renderer.h > > @@ +44,3 @@ > > > > + gboolean (* update_cursor) (MetaCursorRenderer *renderer, > > + MetaCursorSprite *cursor_sprite); > > > > I found it confusing reading the code that: > > > > update_cursor > > > > Updates the internal state of the *renderer* based on the cursor_sprite. > > While > > > > realize_cursor_for_wl_buffer > > > > Updates the state of the *cursor*. > > > > I don't really understand why the change to add the cursor_sprite parameter > > was needed, and I think it contributes to the problem by making the > > non-parallel methods in the interface look more parallel. > > > > I pass the sprite because I don't want to add more uses of > priv->displayed_cursor (explained above). If the tracker is considered to be the canonical place to keep track of the identity and position of the current MetaCursorSprite, it would make more sense for the renderer to *pull* the information it needs from the tracker, rather than to continually pass everything you want in - I can't imagine that you want: void meta_cursor_renderer_set_position (MetaCursorRenderer *renderer, MetaCursorSprite *sprite, int x, int y); void meta_cursor_renderer_set_cursor (MetaCursorRenderer *renderer, MetaCursorSprite *sprite, int x, int y); So adding this parameter seems to me to be going in the wrong direction. But let's try to get the branch landed - I don't think keeping it in-flight longer is doing us any good. [...] > The new version of the big-chunk-patch does this now. It could even more > refactoring, but in summary, drmModeSetCursor2() is now called once per frame > only, and updating the cursor will queue a redraw which will cause calling of > the whole stage to be redrawn (well, not really, because there may be no > damage) with all the drm ioctls with it. Have you tested in detail what happens when only moving the HW cursor? Do we *actually* not paint anything? Do we still page flip? > (In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c116">comment #116</a>) > > (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] [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. > > > > That is kind of why I used that naming. It creates the renderer specific > backing after all the available information is available. But in the future, > I'd like to make this happen just before the drmModeSetCursor2 once per > frame, and avoid creating any unnecessary buffers, but I believe that can > wait. OK. I think things will be cleaned up long-term if we can go in the direction of making the renderer pull necessary information rather than continually poking down to the renderer layer.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>