<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#c108">Comment # 108</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#c105">comment #105</a>) > Review of <a href="attachment.cgi?id=309533&action=diff" name="attach_309533" title="wayland: GObject:ify surface roles">attachment 309533</a> <a href="attachment.cgi?id=309533&action=edit" title="wayland: GObject:ify surface roles">[details]</a> <a href='review?bug=744932&attachment=309533'>[review]</a> [review]: > > Hmm, this patch seems to fall between two things: > > A) An enumeration with methods - this is possible, for example, in Java, I > could have a commit() method that I implement separately for each member of > a role enumeration. > B) A real aggregate object (decorator) that is assigned to the surface to > make it gain new data and behaviors. B) has been the whole purpose of this. Could you point out where I made things as A) describe? I know there are a couple of if (type == X) do this, but didn't want to make the patch too huge. > > Of the two, I like B) better - because we actually have things like /* > xdg_surface stuff */ in the MetaWaylandSurface type. As a long-term > direction, I'd like to see data move to that auxiliary object. (Don't do > that as part of this branch, however :-) That has been a longer term goal with the role object. > > Is MetaWaylandSurfaceRole a good name for the baseclass? I've gone back and > forth on my thinking on this - it's like having a Species class where the > DogSpecies and CatSpecies subclasses have get_name() and get_age() methods - > so certainly wrong at a nit-picky level. But on the other hand - I can't > think that the code is going to get any easier to read if we have > MetaWaylandSurfaceRoleDecorator and XdgSurfaceRoleDecorator rather than > MetaWaylandSurfaceRole and XdgSurfaceRole - I don't think so. So let's go > with this naming. Yea, lets go with just Role. Decorator can be easily confused with graphical decoration which we also do. > > ::: src/wayland/meta-wayland-surface.c > @@ +95,3 @@ > + if (!surface->role) > + surface->role = g_object_new (role_type, NULL); > + else if (G_OBJECT_TYPE (surface->role) != role_type) > > Style: branches of an if statement be consistent - either {} or not. > > @@ +208,3 @@ > + wl_list_insert_list (&surface->compositor->frame_callbacks, > + &pending->frame_callback_list); > + wl_list_init (&pending->frame_callback_list); > > This code definitely shouldn't be duplicated in multiple places ... it's > hard enough to read in one place! Add a helper function. > > @@ +287,3 @@ > + queue_surface_actor_frame_callbacks (surface, pending); > + > + if (META_IS_WAYLAND_SURFACE_ROLE_WL_SHELL_SURFACE (surface->role)) > > Wouldn't it be better if the role type names were just > MetaWlShellSurfaceRole, etc? I suppose it would. I just followed the convention of putting Wayland things behind a MetaWayland. While its nice to be informative in the names, they are indeed a bit long. I'm torn here. > > META_IS_WL_SHELL_SURFACE_ROLE (surface->role) > > @@ +2352,3 @@ > + wl_list_insert_list (&surface->frame_callback_list, > + &pending->frame_callback_list); > + wl_list_init (&pending->frame_callback_list); > > Normalize your GObject code, at the call site: > > if (surface->role) > meta_wayland_surface_role_commit(surface->role, surface->pending); > else > ... > > @@ +2358,3 @@ > +static gboolean > +meta_wayland_surface_role_is_on_output (MetaWaylandSurface *surface, > + MetaMonitorInfo *monitor) > > Same thing here: a function called - > meta_wayland_surface_role_is_on_output() should never be called if the > appropriate MetaWaylandSurfaceRole is null. > > ::: src/wayland/meta-wayland-surface.h > @@ +41,3 @@ > GObject); > > +typedef GType MetaWaylandSurfaceRoleType; > > Please remove this typedef, it makes things more confusing. > > @@ +52,3 @@ > + > + void (*commit) (MetaWaylandSurface *surface, > + MetaWaylandPendingState *pending); > > By not having the MetaWaylandSurfaceRole as the first member, you are > creating a new "language feature" for GObject - don't do this. It makes the > work of the person reading the code harder and also creates unbindable > objects classes. > > @@ +57,3 @@ > +}; > + > +#define META_DECLARE_WAYLAND_SURFACE_ROLE(ROLE_NAME, RoleName, role_name) > \ > > My advice - use your own judgment about whether you want to change the code > or not, is that these macros aren't worth it and make it harder to > understand the code. > > Basically what you've done is replace 6 standard GObject definitions with 3 > complex custom macros, plus code to use them - while you've reduced the > total line count, you've introduced complexity "why is this different from > all the other subclasses?" - oh, because there were 6 of them. > > And when in a follow-up patch you need to open-code the definition of the > cursor role to add a dispose() function, that further increases the > complexity because META_DEFINE_WAYLAND_SURFACE_ROLE() can't even be > considered to be opaque. Yea, I've been rethinking using macros. In the future more and more will not be using them anyway as well, so I'm fine with open coding it. > > @@ +237,3 @@ > + > MetaWaylandSurfaceRoleType role_type, > + struct wl_resource > *error_resource, > + uint32_t > error_code); > > This is really not a setter - setter take a single argument that directly > becomes a property of the object, and cannot fail. > meta_surface_create_and_assign_role() would be a better name. > > I wonder if this would be better open-coded in the setter: > > if (surface->role != NULL) > { > wl_resource_post_error (error_resource, error_code, > "wl_surface@%d already has a different role", > wl_resource_get_id (surface->resource)); > return; > } > > meta_surface_set_role(surface, g_object_new(meta_dnd_surface_get_role(), > NULL)); Maybe even if (meta_wayland_surface_assign_role (surface, ..._get_type()) != 0) wl_resource_post_error (...); Because the role assigning check is not as simple as a NULL check. > > But while that is certainly clearer, and allows separating the check from > the operation if necessary to cleanly not do anything on error, having to > duplicate the error string many times across the code is a pain, so I'm OK > with a renamed (and probably not-int-returning) version of this. What do you mean with not int returning? What should it return then? The caller still needs to know whether the assignment succeeded or not.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>