[Wayland-bugs] [Bug 744932] Wrong (ultra tiny/small) cursor size on HiDPI screen

[mutter (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=744932

Owen Taylor <otaylor at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #309533|none                        |needs-work
             status|                            |

--- Comment #105 from Owen Taylor <otaylor at redhat.com> ---
Review of attachment 309533:

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.

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 :-)

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.

::: 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?

 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.

@@ +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));

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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-bugs/attachments/20150827/0acfbe08/attachment.html>