weird behavior of transient windows in desktop shell in weston

Submitted by Barry Song on April 15, 2019, 9:25 p.m.

Details

Message ID CAGsJ_4yhTFGpipCMKEjYHBB1m81h5NO6KPZh3rLSwJHu3x32Cw@mail.gmail.com
State New
Headers show
Series "weird behavior of transient windows in desktop shell in weston" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Barry Song April 15, 2019, 9:25 p.m.
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@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@navico.com>

                     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

Patch hide | download patch | download mbox

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,