<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#c39">Comment # 39</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#c37">comment #37</a>) > (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] [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 Note that the x11 backend here behaves differently depending on the running mode which is why I have been suggesting backends/x11/[cm/nested]. > > 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. We have already started separating backends from the rest, so I think in that case the abstraction can be pretty straight forward (see my comment above). > > 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! > I don't think "winsys" is good term for it either. We'd have different "winsys" meaning different things depending on the layer (clutter/cogl vs mutter). The reason MetaCursorSpriteWayland is an awkward fit is because it's a mix of Wayland client interaction logic and running mode logic. So maybe a better design would be to split those into different units. > 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! That might be because when an X CM, the cat lives in the X server, but in Wayland we are the 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? This is why I put it in the Wayland object; it only ever makes sense do do it there. > - 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. I'd have to think about this more. I agree its not an optimal design as it blurs the line between "running mode" and the protocol used. (In reply to Jasper St. Pierre from <a href="show_bug.cgi?id=744932#c38">comment #38</a>) > The frontend needs a way of specifying "a cursor" (a wl_surface, an X cursor > name), and the backend needs a way of accepting that and saying "I prefer a > wl_surface" or "I prefer an X cursor name", because accepting the other > might be costly. > > There's conceptually two separate objects, one frontend (the protocol object > the user manipulates) and one backend (the object that is being displayed on > the screen), but in practice they're not different enough to be two > different objects. That's why it's sort of in this weird middle-ground right > now. I already separated the backend from the frontend completely (all backend related code is now in MetaCursorRenderer*). Its rather "meta_is_wayland_compositor ()" or "running mode" or whatever to call it. Maybe we should have such a abstraction, not calling it Wayland but "display server", and then hook wl_surface etc to it some other way and put it under a "protocols/" directory. Not sure what to call that though; its neither "winsys", "protocol" or "frontend". I.e. something like backends/ x11/ cm/ nested/ protocols/ wayland/ x11/ xwayland/ (funky mix of the two above) "running mode"/ x11_cm/ display_server/ ? MetaCursorSprite would go under "running mode"/ and under protocols we'd have a MetaWaylandCursor which interacts with MetaCursorSprite.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>