<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#c37">Comment # 37</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#c34">comment #34</a>) > (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] [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. So: Wayland running mode: - With *both* wayland and X protocols to talk to clients - With *either* native or x11 backend X11 compositor: - With X protocol to talk to client - With x11 backend Code might be X11 code because it's about a window that we are going to act as a X11 window manager for (X window or Xwayland window), it might be X11 code because it's handling X input events (X compositor or nested Wayland), it might be X11 code because it's specific to when we are running as a X compositor. This is obviously pretty hard to represent either with a directory hierarchy or with a class hierarchy. I think trying to be too strict with how we organize things is going to be confusing, which could encourage something like a winsys/ directory (I guess this is the same as our current x11/ and wayland/ directories in the toplevel.) I'm not sure I believe in a strict "frontend protocol" directory, because it seems like the first thing that you wanted to put in there doesn't really match that directory! That all being said I have some specific concerns about MetaCursorSprite vs. MetaCursorSpriteWayland. MetaCursorSprite, as I understand it, is basically similar to MetaCursorReference - it's basically a union of different ways to specify the appearance of a cursor. A wayland-specific subclass of this might make sense so that it can store along with the appearance a pointer back to a Wayland object. But a Wayland specific subclass shouldn't gain entirely new capabilities like looking going out and looking at where it is relative to the monitors and reloading the theme image at a different size. I feel like the parent class is fur, and the subclass is a cat! Things to think about for improving the design: - Do some pieces (like loading from the theme at a different size) work better in the generic code, even if they are only ever used in the Wayland case? - Can meta_cursor_sprite_wayland_update_position() be modified to use only public functionality and then live somewhere else in the code, perhaps as a procedural method? As I said before, if you think this is the best way to organize it, I'm willing to just review it as is - sometimes there's no clean design that actually works out.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>