[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

--- Comment #98 from Jonas Ådahl <jadahl at gmail.com> ---
(In reply to Owen Taylor from comment #95)
> Review of attachment 309854 [details] [review]:
> 
> I attached this patch for review as part of this branch... but looking at
> it, I'd definitely like it if we can figure out a way to separate it - it
> looks entirely unrelated to the idea of the branch, and I'm not convinced by
> it as a patch yet. Is there some way we can land this branch without this>
> 
>  * It's not clear to me why Xwayland windows are a separate role... they
> seem to behave a lot like shell surfaces, and in a brief look it appears
> Xwayland calls wl_shell_get_shell_surface() on them - which by the protocol
> makes them have the shell surface role. I don't particularly like
> introducing the idea that we have internal pseudo-roles that are different
> from the roles of the Wayland protocol. If we want to special-case Xwayland
> windows, shouldn't we do it by some other method?

A "role" is meant to define what a wl_surface does. We already have a set of
roles for all "ways" a wl_surface behaves - except XWayland wl_surfaces. We
have the three different toplevel surface roles (wl_shell_surface, xdg_surface,
xdg_popup), the subsurface role, cursor role and DND role. To me it simply only
makes sense to introduce a XWayland role for wl_surface's that represent an
XWindow, because they, like all the other roles, need to do things differently
from all the other roles.

If we special case the XWayland "role" (or whatever to call it if we cant use
that name in the implementation), then we'd need to have "if (have_role)
do_role_specific_thing() else if (is_xwayland_window)
do_xwayland_specific_thing()" which doesn't make sense at all to me.

I will insist we treat XWayland window wl_surface's as roles internally. If
we'd had a wl_xwayland protocol, we could have a
wl_xwayland_get_xwayland_surface that assigns such a role, but the code would
look exactly the same in mutter for all other parts except when to assign the
role. If you really don't want to call it role, we could s/role/something_else/
but I would still just assign something_else when the protocol defines we
should assign a role.

I suppose one could say part of the problem is that XWayland - Wayland
"protocol" is in no way specified, and we don't really have a place to specify
the role-ness of how it works. I did contemplate whether it would make sense to
have a wl_xwayland protocol just for the role assigning, but decided not to do
that unless there are more things needed from such a protocol.

> 
>  * The commit message here doesn't reflect the idea of the patch at all,
> because looking at the huge comment in meta-wayland-surface.c, it looks like
> the *idea* of the patch is that we want to somehow treat frame callbacks
> differently on Xwayland windows to try and fix visual artifacts during map?

The huge comment just describes what was already done before. We kind of need
to ignore the protocol here, we already do, and this patch just documents this
and the reason why it is needed. The actual point of the patch is not really to
describe reality though, but to introduce the role so we can deal with the
wl_surface in the same way as with other wl_surface's.

> 
> ::: src/wayland/meta-xwayland.c
> @@ +49,3 @@
> +                                     META_WAYLAND_SURFACE_ROLE_XWAYLAND,
> +                                     surface->resource,
> +                                     WL_DISPLAY_ERROR_INVALID_OBJECT))
> 
> Hmmm ... I'm quite surprised that meta_wayland_surface_set_role() doesn't
> follow Mutter standards for error handling, but returns 0/-1... I'd
> definitely write this as:
> 
>  != 0
> 
> as other callers do to make it clearer what is going on.
> 
> (But please don't add more functions like this, and probably this one would
> be better changed.)

Sure. I wanted to differentiate it from an actual server side error, but I can
make it an int().

-- 
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/20150825/773672cf/attachment.html>