KRunner Wayland issues on Qt 5.9

[Martin Flöser]

Am 2017-07-12 18:35, schrieb David Edmundson:
> On Wed, Jul 12, 2017 at 4:02 PM, Martin Flöser <mgraesslin at kde.org>
> wrote:
> 
>> Hi all,
>> 
>> I had a look at the issues of KRunner on Wayland with Qt 5.9 and
>> want to share my findings.
>> 
>> What we see is KRunner working fine on first invokation, but
>> afterwards everything is messed up:
>> * maximized windows shortly change their size
>> * KRunner opens behind other windows
>> 
>> With WAYLAND_DEBUG I was able to understand the issue. On first
>> invokation everything is normal:
>> * Plasma shell surface gets created
>> * Plasma shell surface gets positioned
>> * Plasma shell surface gets marked as panel
>> * Panel is set to windows go below
>> * Panel is marked as accepts focus
>> * Surface gets mapped
>> 
>> Yay, a situation KWin can handle.
>> 
>> Now the window gets hidden, and shown again and Qt recreates the
>> surface. Due to queuing we have
>> 1. Surface gets mapped
>> 2. KWin shows the window at random position
>> 3. Plasma shell surface gets created and marked as panel
>> 4. KWin adjust it to be a panel and moves maximized windows around
>> 5. Plasma shell surface gets marked as panel with windows go below
>> 6. KWin adjust it to be a panel with windows go below and moves
>> maximized windows around
>> 7. Panel gets marked as accepts focus (too late, too late)
>> 8. KRunner gets positioned
>> 
>> So we are in a bad state. When looking at it our Plasma Shell
>> Surface protocol is not double buffered which explains why KWin
>> reacts on each and every of the actions. It was designed so that
>> double buffered is not needed as it was supposed to be used before
>> mapping the window.
>  > Any ideas or advice?
> 
> I can do inane rambling instead.
> 
> ----
> 
> With Xdgv6 (in theory!) this ends up being a non-issue.
> 
> A client is not allowed to attach a buffer until it gets a configure
> event from the server.
> 
> This throws in an extra roundtrip and according to my logs on the
> second showing that means we do get all the plasma shell stuff sent
> before we send a configure which means we'll have processed them all
> before we get the first buffer and map the window.

this is extremely good! That will solve many many issues we have with 
initial state handling.

> 
> In practice we get the role and location, but not the position (which
> means I think I now get what Marco's wayland krunner was trying to
> do.)

The position is not that much of an issue as long as KWin gets it before 
KWin renders.

> 
> ---
> 
> We should be double buffering the proper shells (wl_shell, xdgV5/6)
> and only emitting the surfaceCreated signals when the surface is
> comitted.

IIRC wl_shell is not defined to be double buffered. Not sure how it's 
about xdg. There is always the chance of breaking things 
*cough*Qt*cough* when changing that - even if it is sane.

What could be an idea is to not do anything in KWin till we get the 
first map event. That is evaluate all the shell options only on map.

> 
> In practice that won't acheive anything as QWaylandWindow::initWindow
> calls commit a few times randomly before the expose event that we're
> currently hooking into is called. Also kwayland-integration calls
> commit on the surface every time it attaches something, which I don't
> think is right.

It is fine given the definition of the interfaces. We setup some state 
and commit it. If we don't commit we don't know when that will happen. 
We only adjusted our own relevant states and everything else is not 
affected.

> 
> --
> 
>> Now with Qt's new behavior that bites our ass and I'm a little bit
>> lost what to do. I think we can improve the situation by first
>> setting the panel behavior, but the wrong position is something we
>> cannot prevent if the window is already mapped. Even changing the
>> protocol to be double buffered would not really help here as we are
>> too late. It's not every frame is perfect.
> 
> I do think we can change Qt's behaviour. With XdgV7 it's clarified
> that there really is no need to delete the xdg_toplevel to unmap.

I hope they believe us now on this point...

> 
> That still leaves Qt having the "new behaviour" if one calls
> QWindow::setParent or QWindow::setType, but from our POV that's fine.

well setType is a bit problematic for plasma-integration (windeco, 
etc.), but at least for Plasma surfaces it's fine.

Cheers
Martin