<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#c34">Comment # 34</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#c30">comment #30</a>) > Review of <a href="attachment.cgi?id=299282&action=diff" name="attach_299282" title="MetaCursorSprite: Move Wayland part to own its frontend implementation">attachment 299282</a> <a href="attachment.cgi?id=299282&action=edit" title="MetaCursorSprite: Move Wayland part to own its frontend implementation">[details]</a> <a href='review?bug=744932&attachment=299282'>[review]</a> [review]: > > If the point of the refactoring is to get to a clean design, there are > elements of this patch that feel off to me > > ::: src/frontends/meta-cursor.c > @@ +156,2 @@ > MetaCursorSprite * > +meta_cursor_sprite_new (void) > > meta_foo_new() should create a MetaFoo, not a MetaFoo or a subclass. This is > something like meta_create_cursor_sprite(). Then meta_create_cursor_sprite() it is then. > > @@ +160,3 @@ > + return g_object_new (META_TYPE_CURSOR_SPRITE_WAYLAND, NULL); > + else > + return g_object_new (META_TYPE_CURSOR_SPRITE, NULL); > > Hmm, I think this reflects confusion whether MetaCursorSprite is a frontend > or a backend object. If it's genuinely a "frontend" object, then when > creating it we should always know what protocol we are creating it for. And > meta_cursor_sprite_from_theme() would be a third subclass independent of the > backend. MetaCursorSprite is a kind of a frontend/protocol thing. Some confusion is that while we always have one backend, we have multiple frontends/protocols but one "running mode" (i.e. Wayland compositor or X11 CM), so really MetaCursorSprite is always a MetaCursorSpriteWayland when running as a Wayland compositor. I've been thinking of organising things in some way that reflects that but haven't come up with any good idea yet. > > Obviously every transformation of frontend to backend isn't going to be > possible - e.g., creating an X cursor for a Wayland surface - but I don't > think that's a big problem - you just need to have the code somewhere that > knows what can be transformed to what. > > ::: src/frontends/wayland/meta-cursor-wayland.c > @@ +66,3 @@ > + MetaCursorSpriteWayland *self; > + > + self = META_CURSOR_SPRITE_WAYLAND (meta_cursor_sprite_new ()); > > This doesn't make sense to me - this *has* to be a MetaCursorSpriteWayland, > so just create one. The idea was to centralize creation of a MetaCursorSprite* but I can remove that centralization if you prefer.(In reply to Owen Taylor from <a href="show_bug.cgi?id=744932#c31">comment #31</a>) > Review of <a href="attachment.cgi?id=301060&action=diff" name="attach_301060" title="wayland: Paint proper pointer cursor size on HiDPI monitors">attachment 301060</a> <a href="attachment.cgi?id=301060&action=edit" title="wayland: Paint proper pointer cursor size on HiDPI monitors">[details]</a> <a href='review?bug=744932&attachment=301060'>[review]</a> [review]: > > Hmm, this deviates a lot from how I'd expect things to be organized. Use > your judgement whether you agree with me or not :-) If you want me to review > things as is without any reconceptualization of MetaCursorSprite , let me > know, and I'll do that. > > ::: src/backends/meta-cursor-renderer.c > @@ -103,3 @@ > - priv->current_rect.y = priv->current_y - hot_y; > - priv->current_rect.width = cogl_texture_get_width (COGL_TEXTURE > (texture)); > - priv->current_rect.height = cogl_texture_get_height (COGL_TEXTURE > (texture)); > > I'd have expected the rectangle computations to stay here and the > MetaCursorSprite operation to be more like "here's the desired scale, what > do you have for me?" The resulting rectangle is not only used by the renderer, it is also used to detect what outputs the cursor wl_surface is on (see meta-wayland-surface.c in a later patch). In other words, since MetaCursorSpriteWayland sometimes can be a wl_surface, we need to treat it more as such. > > ::: src/frontends/wayland/meta-cursor-wayland.c > @@ +129,3 @@ > + image_scale = 1; > + if (priv->monitor_scale != monitor->scale) > + meta_cursor_sprite_load_from_theme (cursor_sprite, monitor->scale); > > This is certainly not where I'd expect to find this code... in the pure > "frontend" conceptualization of MetaCursorSprite, a MetaCursorSpriteWayland > has nothing to do with themed cursors - a wayland cursor is one set by a > Wayland client. A MetaCursorSpriteWayland is the MetaCursorSprite implementation when running as a Wayland compositor. Again, this confusion is probably that we don't have a "running mode" abstraction, only the "meta_is_wayland_compositor ()" thing which is not very nice.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>