Bug 384462 - Maximize effect does not apply to wayland clients
Summary: Maximize effect does not apply to wayland clients
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: effects-various (show other bugs)
Version: git master
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
: 390185 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-09-07 11:00 UTC by Kai Uwe Broulik
Modified: 2018-10-06 17:35 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:
mgraesslin: Wayland+
mgraesslin: X11-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kai Uwe Broulik 2017-09-07 11:00:48 UTC
When maximizing a Wayland client it is not animated. X clients animate just fine. When doing so I noticed that the order and amount of signals emitted is different for the two:

Maximizing an X client:
Shape changed old was QRect(0,0 682x512)
Shape changed old was QRect(0,0 1024x768)
Shape changed old was QRect(0,0 682x512)
Maximized changed true true

Maximizing a Wayland client:
Shape changed old was QRect(342,260 682x512)
Maximized changed true true
Shape changed old was QRect(342,260 682x512)
Comment 1 Kai Uwe Broulik 2017-09-07 11:05:56 UTC
Output with new geometries:

X client
Shape changed old was QRect(432,232 682x512) now is QRect(432,232 682x508)
Shape changed old was QRect(0,0 1024x768) now is QRect(0,0 1024x768)
Shape changed old was QRect(432,232 682x512) now is QRect(0,0 1024x768)
Maximized changed true true

Wayland client:
Shape changed old was QRect(251,350 1024x768) now is QRect(251,350 1024x768)
Maximized changed true true
Shape changed old was QRect(251,350 1024x768) now is QRect(0,0 1024x768)

(this is the output from when double clicking the title bar to maximize, I do get geometry changed events when moving the window, obviously, but in the instance of maximizing there's some superfluous events)
Comment 2 Martin Flöser 2017-09-07 15:14:58 UTC
This used to work in the past, given that I assume it's another Qt 5.9 regression.

I'm not going to investigate prior to xdg shell v6 support landed.
Comment 3 Martin Flöser 2018-01-01 17:15:17 UTC
We might have to fork the effect and have it work differently for X11 and Wayland windows.
Comment 4 Martin Flöser 2018-02-10 06:07:40 UTC
*** Bug 390185 has been marked as a duplicate of this bug. ***
Comment 5 Vlad Zahorodnii 2018-03-12 16:33:18 UTC
(In reply to Martin Flöser from comment #3)
> We might have to fork the effect and have it work differently for X11 and
> Wayland windows.

IMO, that's wrong. Maximize effect do scaling and translation in a virtual space, that's all! It doesn't need to know whether it's Wayland or X11 or whatever.

The root of the problem is manual tracking of window geometries. KWin knows what previous geometry was so it should provide it to the effect. I'd taken this approach in https://phabricator.kde.org/D9391

Beside, the maximize effect tracks all geometry changes, that's totally wrong, right?

Another problem is that in some cases KWin changes geometry multiple times behind scenes. This leads to "visual artifacts".

Yet another problem is cross fading. "Note: the window pixmap is not kept forever even when referenced". This is a broken contract between effects and core KWin. https://github.com/KDE/kwin/blob/master/libkwineffects/kwineffects.h#L2211
Also, it doesn't work on Wayland.


In the best scenario, each window should have a state log so effects could look up previous state and do whatever they want. E.g. 

auto currentGeo = w->stateLog()->current().geometry();
auto prevGeo = w->stateLog()->previous().geometry();
morphGeometry(w, prevGeo, currentGeo);

But this would be really big change also it's not really clear how to handle state log(e.g. how to merge states, etc). Yet, I wish KWin had this.


I suggest to change windowMaximizedStateChanged's prototype to

void windowMaximizedStateChanged(EffectWindow *w, const MaximizeChangedData &data);

struct MaximizeChangedData {
    bool vertically;
    bool horizontally;
    QRect oldGeometry;
    QRect newGeometry;
};

so effects have at least minimum required information. Also, it would break compatibility with third-party effects. But if I were asked to choose between working effects and compatibility I would choose "workking effects".
Comment 6 Martin Flöser 2018-03-18 08:05:15 UTC
(In reply to Vlad Zagorodniy from comment #5)
> (In reply to Martin Flöser from comment #3)
> > We might have to fork the effect and have it work differently for X11 and
> > Wayland windows.
> 
> IMO, that's wrong. Maximize effect do scaling and translation in a virtual
> space, that's all! It doesn't need to know whether it's Wayland or X11 or
> whatever.

The reason I wrote that is a conceptional difference between X11 and Wayland when maximizing windows.

On X11:
 * window manager resizes window
 * window manager sets the maximized state

On Wayland:
 * window manager sets state to maximized
 * window manager sends a configure request to the window with the new state and the new geometry
 * window renders to new size and only at that time the geometry changes

Ideally we would only apply the state change when the new size is acknowledged. But that's something KWin cannot do and something I doubt is worth the effort.

And that's what's breaking the maximize effect. When it reacts on the maximized changed signal, the window doesn't have the right geometry yet.

> 
> The root of the problem is manual tracking of window geometries. KWin knows
> what previous geometry was so it should provide it to the effect. I'd taken
> this approach in https://phabricator.kde.org/D9391
> 
> Beside, the maximize effect tracks all geometry changes, that's totally
> wrong, right?

Well it has to for the problem like on Wayland that maximize and geometry change are not synced.

> 
> Another problem is that in some cases KWin changes geometry multiple times
> behind scenes. This leads to "visual artifacts".

That shouldn't happen. The behind the scene changes should have geometry update blockers, so that in the end there is only a change from old to new geometry.

> 
> Yet another problem is cross fading. "Note: the window pixmap is not kept
> forever even when referenced". This is a broken contract between effects and
> core KWin.

The contract is that there is no guarantee. So the contract cannot be broken.

> https://github.com/KDE/kwin/blob/master/libkwineffects/kwineffects.h#L2211
> Also, it doesn't work on Wayland.

It should work on Wayland as well ;-) I have seen it working in the past.

> 
> 
> In the best scenario, each window should have a state log so effects could
> look up previous state and do whatever they want. E.g. 
> 
> auto currentGeo = w->stateLog()->current().geometry();
> auto prevGeo = w->stateLog()->previous().geometry();
> morphGeometry(w, prevGeo, currentGeo);
> 
> But this would be really big change also it's not really clear how to handle
> state log(e.g. how to merge states, etc). Yet, I wish KWin had this.

I think that is not possible.

> 
> 
> I suggest to change windowMaximizedStateChanged's prototype to
> 
> void windowMaximizedStateChanged(EffectWindow *w, const MaximizeChangedData
> &data);
> 
> struct MaximizeChangedData {
>     bool vertically;
>     bool horizontally;
>     QRect oldGeometry;
>     QRect newGeometry;
> };
> 
> so effects have at least minimum required information. Also, it would break
> compatibility with third-party effects. But if I were asked to choose
> between working effects and compatibility I would choose "workking effects".

The old signal could be kept for compatibility.

I'm not sure that sending this information would really help the situation for the Wayland case.
Comment 7 Vlad Zahorodnii 2018-03-18 14:37:30 UTC
(In reply to Martin Flöser from comment #6)
> The reason I wrote that is a conceptional difference between X11 and Wayland
> when maximizing windows.
> 
> On X11:
>  * window manager resizes window
>  * window manager sets the maximized state
> 
> On Wayland:
>  * window manager sets state to maximized
>  * window manager sends a configure request to the window with the new state
> and the new geometry
>  * window renders to new size and only at that time the geometry changes
> 
> Ideally we would only apply the state change when the new size is
> acknowledged. But that's something KWin cannot do and something I doubt is
> worth the effort.
> 
> And that's what's breaking the maximize effect. When it reacts on the
> maximized changed signal, the window doesn't have the right geometry yet.

> Well it has to for the problem like on Wayland that maximize and geometry
> change are not synced.

> The old signal could be kept for compatibility.
> 
> I'm not sure that sending this information would really help the situation
> for the Wayland case.

Oh, I was thinking that KWin hides the xdg shell protocol specifics. Silly me.

As you see the maximize effect is exposed too much to low-level details.
What about if a next version of the xdg shell protocol totally breaks compatibility with
the previous ones? [It most likely won't happen but let's imagine it could.]
That would break effects!

Anyways, it would be great to send that information to effects because in most cases
effects are "abstract" (they do affine transformations, etc) and don't care what
platform it is, etc. Beside that, supplying effects with all they need makes them less
error prone.
Comment 8 Patrick Silva 2018-06-12 22:08:40 UTC
Here wayland clients are animated if I drag the window up/down to maximize/unmaximize it.
There is no animation when I click the buttons in the window decoration or double click the window decoration.

plasma 5.13, frameworks 5.47 and qt 5.11 on Arch Linux.