weird behavior of transient windows in desktop shell in weston

[Barry Song]

ping

Barry Song <21cnbao at gmail.com> 于2019年4月17日周三 下午10:19写道：
>
> Hi guys,
>
> I got some very weird result while using transient window in weston.
> Let's start from a simplest qt program:
>
> int main(int argc, char *argv[])
> {
>     QApplication a(argc, argv);
>     MainWindow w;
>     w.show();
>
>     QDialog d1(&w)
>     d1.setWindowTitle("transient& non toplevel dialog d1");
>     d1.show();
>
>     return a.exec();
> }
> Under x11 or mutter wayland compositor, D1 transient window will
> always be on the top of Mainwindow W. But under weston desktop shell,
> if we click mainwindow, d1 will be covered by mainwindow W.
>
> I tried to read source code to figure out why and found weston will
> only think WL_SHELL_SURFACE_TRANSIENT_INACTIVE transient windows as
> "TRANSIENT", otherwise it will think it as "TOPLEVEL" window.
> code path: libweston-desktop/wl-shell.c
>
> static void
> weston_desktop_wl_shell_surface_protocol_set_transient(struct
> wl_client *wl_client,
>                                                        struct
> wl_resource *resource,
>                                                        struct
> wl_resource *parent_resource,
>                                                        int32_t x, int32_t y,
>                                                        enum
> wl_shell_surface_transient flags)
> {
>         struct weston_desktop_surface *dsurface =
>                 wl_resource_get_user_data(resource);
>         struct weston_surface *wparent =
>                 wl_resource_get_user_data(parent_resource);
>         struct weston_desktop_surface *parent;
>         struct weston_desktop_wl_shell_surface *surface =
>                 weston_desktop_surface_get_implementation_data(dsurface);
>
>         if (!weston_surface_is_desktop_surface(wparent))
>                 return;
>
>         parent = weston_surface_get_desktop_surface(wparent);
>         if (flags & WL_SHELL_SURFACE_TRANSIENT_INACTIVE) {
>                 weston_desktop_wl_shell_change_state(surface, TRANSIENT, parent,
>                                                      x, y);
>         } else {
>                 weston_desktop_wl_shell_change_state(surface, TOPLEVEL, NULL,
>                                                      0, 0);
>                 surface->parent = parent;
>                 weston_desktop_api_set_parent(surface->desktop,
>                                               surface->surface, parent);
>         }
> }
>
> Actually WL_SHELL_SURFACE_TRANSIENT_INACTIVE is pretty difficult to be
> true unless we put Qt::WindowTransparentForInput or Qt::ToolTip for a
> qt window.
>
> On the other hand, mutter doesn't check this flag at all:
>
> code path: mutter/src/wayland/meta-wayland-wl-shell.c
> static void
> wl_shell_surface_set_transient (struct wl_client   *client,
>                                 struct wl_resource *resource,
>                                 struct wl_resource *parent_resource,
>                                 int32_t             x,
>                                 int32_t             y,
>                                 uint32_t            flags)
> {
>   MetaWaylandWlShellSurface *wl_shell_surface =
>     META_WAYLAND_WL_SHELL_SURFACE (wl_resource_get_user_data (resource));
>   MetaWaylandSurface *surface =
>     surface_from_wl_shell_surface_resource (resource);
>   MetaWaylandSurface *parent_surf = wl_resource_get_user_data (parent_resource);
>
>   wl_shell_surface_set_state (surface,
>                               META_WL_SHELL_SURFACE_STATE_TRANSIENT);
>
>   set_wl_shell_surface_parent (surface, parent_surf);
>   wl_shell_surface->x = x;
>   wl_shell_surface->y = y;
>
>   if (surface->window && parent_surf->window)
>     sync_wl_shell_parent_relationship (surface, parent_surf);
> }
>
> I seem mutter is doing right things just like a typical x11 window manager.
>
> On the other hand, the code path of
> WL_SHELL_SURFACE_TRANSIENT_INACTIVE can easily crash as I have sent
> the patch yesterday:
> commit e87c23ffcf3dc209cb8e7f24c3087636b8db3f53 (HEAD -> 6.0)
> Author: Barry Song <barry.song at navico.com>
> Date:   Mon Apr 15 11:28:07 2019 +1200
>
>     desktop-shell: fix the crash while clicking TRANSIENT_INACTIVE window
>
>     It is pretty easy to replicate this bug by involving a Qt Window with
>     Qt::WindowTransparentForInput flag.
>
>     int main(int argc, char *argv[])
>     {
>         QApplication a(argc, argv);
>         MainWindow w;
>         w.show();
>
>         QDialog d1(&w, Qt::WindowTransparentForInput);
>         d1.show();
>
>         return a.exec();
>     }
>
>     Click d1 dialog, weston will crash.
>
>     Signed-off-by: Barry Song <barry.song at navico.com>
>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 93b1c70b..9473bac1 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -2697,11 +2697,16 @@ desktop_surface_move(struct
> weston_desktop_surface *desktop_surface,
>         struct weston_touch *touch = weston_seat_get_touch(seat);
>         struct shell_surface *shsurf =
>                 weston_desktop_surface_get_user_data(desktop_surface);
> -       struct weston_surface *surface =
> -               weston_desktop_surface_get_surface(shsurf->desktop_surface);
> -       struct wl_resource *resource = surface->resource;
> +       struct weston_surface *surface;
> +       struct wl_resource *resource;
>         struct weston_surface *focus;
>
> +       if (!shsurf)
> +               return;
> +
> +       surface = weston_desktop_surface_get_surface(shsurf->desktop_surface);
> +       resource = surface->resource;
> +
>         if (pointer &&
>             pointer->focus &&
>             pointer->button_count > 0 &&
>
> I am also trying to figure out a possible way to make weston do same
> work as mutter, and a possible way is adding set_parent() callback and
> maintaining the relationship of so-called "TOPLEVEL" transient windows
> in set_parent():
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 9473bac1..9b4cf46c 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -2689,6 +2689,13 @@ set_fullscreen(struct shell_surface *shsurf,
> bool fullscreen,
>         weston_desktop_surface_set_size(desktop_surface, width, height);
>  }
>
> +static void
> +desktop_surface_set_parent(struct weston_desktop_surface *surface,
> +                            struct weston_desktop_surface *parent,
> +                            void *user_data)
> +{
> +}
> +
>  static void
>  desktop_surface_move(struct weston_desktop_surface *desktop_surface,
>                      struct weston_seat *seat, uint32_t serial, void *shell)
> @@ -2933,6 +2940,7 @@ static const struct weston_desktop_api
> shell_desktop_api = {
>         .surface_removed = desktop_surface_removed,
>         .committed = desktop_surface_committed,
>         .move = desktop_surface_move,
> +       .set_parent = desktop_surface_set_parent,
>         .resize = desktop_surface_resize,
>         .fullscreen_requested = desktop_surface_fullscreen_requested,
>         .maximized_requested = desktop_surface_maximized_requested,
> @@ -3817,7 +3825,7 @@ activate(struct desktop_shell *shell, struct
> weston_view *view,
>
> I'd like to get some comment from weston maintainers. Is looking
> transient window as toplevel window the expected behavior in weston?
> Or it is better to maintain the layer relationship of transient window
> just like a typical X11 window manager?
>
> Thanks
> Barry