[RFC] [effects] Make Scale and Glide effects Wayland-friendly
ClosedPublic

Authored by zzag on Aug 28 2018, 11:14 AM.

Details

Summary

The Scale effect and the Glide effect have to animate only ordinary
windows(i.e. the ones that are considered to be apps).

On X11, in order to distinguish ordinary windows from combo box popups,
popup menus, and other popups, those effects check whether given window
is managed.

On Wayland, there is no concept of managed/unmanaged windows.

XDG Shell protocol defines 2 surface roles:

  • xdg_toplevel;
  • and, xdg_popup.

The former can be used to implement typical windows, the ones that can
be minimized, maximized, etc.

The latter can be used to implement tooltips, popup menus, etc. Thus,
that's a good criteria to filter popup windows.

CCBUG: 398100

Diff Detail

Repository
R108 KWin
Branch
effects-scale-glide-wayland
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3477
Build 3495: arc lint + arc unit
zzag created this revision.Aug 28 2018, 11:14 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 28 2018, 11:14 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 28 2018, 11:14 AM
zzag added a comment.EditedAug 28 2018, 11:14 AM

We probably also have to support wl_shell_surface. I'm not sure how to name methods in that case. We already have methods like

  • isClient
  • and, isDeleted

So, if there is isToplevel method, then it's name will be deluding a little bit.

EDIT:

Hm, maybe,

  • isToplevelWindow
  • isPopupWindow
zzag planned changes to this revision.Aug 28 2018, 11:23 AM

Concept, yes definitely.

Code wise:
ShellClient implements ShellClient::m_windowType with the X style window type
This is used for internal surfaces and plasma surfaces.

Adding a second layer specially for wayland windows will break all of that.

I think what's needed is setting that m_windowType to popup/whatever as best as we can from the XdgShell as it affects all geometry/focus logic too.

Edit: I got distracted by window types and didn't mention isManaged.

Effectively the documentation for isManaged is true for popups.

 /* Whether this EffectWindow is managed by KWin (it has control over its placement and other
* aspects*/

technically kwin now does position it, but only within the constraints given to us by the application, so to all intents and purposes it doesn't.

It could be introduces as a new property with a new name that wraps both isXdgPopup || isManaged

But certainly I see no reason to expose isX11/isWayland/isTopLevel.

zzag added a comment.EditedAug 28 2018, 11:51 AM

Edit: I got distracted by window types and didn't mention isManaged.

Effectively the documentation for isManaged is true for popups.

 /* Whether this EffectWindow is managed by KWin (it has control over its placement and other
* aspects*/

technically kwin now does position it, but only within the constraints given to us by the application, so to all intents and purposes it doesn't.

It could be introduces as a new property with a new name that wraps both isXdgPopup || isManaged

But certainly I see no reason to expose isX11/isWayland/isTopLevel.

So, what about adding isPopupWindow to EffectWindow?

For Client, it will be the same as isManaged.
For ShellClient, it will return true if corresponding surface is a xdg_popup (or whether wl_shell_surface is a popup).

Something like that.

Though we want a property name that's about how it's "self-positioning" rather than necessarily being about being a popup.

zzag added a comment.Aug 28 2018, 2:46 PM

Though we want a property name that's about how it's "self-positioning" rather than necessarily being about being a popup.

Yeah, assumption that all O-R windows are popups is wrong.

Could we fix that by adding isPopupWindow to Unmanaged?

bool Unmanaged::isPopupWindow() const
{
    return isTooltip()
        || isComboBox()
        || ...;
}

So, effects would do

// A popup can be a normal window (e.g. combo box popup on X11).
if (w->isPopupWindow()) {
    return false;
}

// Comment describing why we don't want to animate unmanaged windows.
if (w->isX11Client() && !w->isManaged()) {
    return false;
}

return w->isNormalWindow()
    || w->isDialog();

It's longer than

// Don't animate popups and unmanaged windows.
if (w->isSelfPositionedWindow()) {
    return false;
}

return w->isNormalWindow()
    || w->isDialog();

but with the isPopupWindow method, we have more readable code and we don't need to introduce shady terminology that puts unmanaged clients on X11 and popups on Wayland under single umbrella.

Also, it makes possible to split the Fade effect.

zzag updated this revision to Diff 42105.Sep 21 2018, 4:31 PM

Alternative approach

zzag added a comment.Sep 21 2018, 4:33 PM

I'm still not sure about tooltips. Looks like they are not popups on Wayland. Will it be changed in Qt?

zzag abandoned this revision.Sep 22 2018, 10:27 AM

Let's wait for the proper tooltip support and then start figuring out how to make the effects work with wayland clients.

I'm still not sure about tooltips. Looks like they are not popups on Wayland. Will it be changed in Qt?

To clarify this. Qt currently (should!) have tooltips as popups.

The only twist is that wayland forces popups to have a parent. If none is set by the client it will make an attempt at guessing based on the focus window and otherwise will fall back to creating the tooltip as a toplevel. This last part is normally obvious as the tooltip will appear nowhere near the place it's providing a tooltip for. This can only really be fixed in each respective client.

In principle I'm fully happy with what this patch is doing, but clearly we have some stuff to figure out.

zzag added a comment.Sep 22 2018, 7:57 PM

To clarify this. Qt currently (should!) have tooltips as popups.

I guess then that's a bug:

zzag reclaimed this revision.Oct 4 2018, 7:36 AM
zzag planned changes to this revision.

Check only whether given window is a popup, drop isX11Client/isWaylandClient.

zzag updated this revision to Diff 42838.Oct 4 2018, 9:08 AM

Use only isPopupWindow

zzag updated this revision to Diff 42839.Oct 4 2018, 9:09 AM

Delete unrelated whitespace change

zzag edited the summary of this revision. (Show Details)Oct 4 2018, 9:15 AM
graesslin accepted this revision.Oct 5 2018, 5:33 PM
This revision is now accepted and ready to land.Oct 5 2018, 5:33 PM

I know you have a ship it, but I'd quite like it if we did as per the bug report discussion with combining both isManaged and isPopup. We can set ShellClient::isClient to true so that we always hit the isPopup check.

Also, I think we need to cache isPopup in deleted.

zzag added a comment.Oct 6 2018, 10:33 AM

I know you have a ship it, but I'd quite like it if we did as per the bug report discussion with combining both isManaged and isPopup. We can set ShellClient::isClient to true so that we always hit the isPopup check.

I tested Krita on X11 and it has some normal unmanaged windows, so, yeah, we have to check whether the window is managed.
Will it be a good idea to override isClient? I personally would prefer to have an X11-specific check, i.e. what the previous version of this revision did.

zzag updated this revision to Diff 43155.Oct 8 2018, 6:59 PM

Add X11-specific check

zzag updated this revision to Diff 43156.Oct 8 2018, 7:05 PM

Fix typos

zzag updated this revision to Diff 43157.Oct 8 2018, 7:12 PM

Edit comments

davidedmundson accepted this revision.Oct 8 2018, 8:41 PM

I don't see why we shouldn't set ShellClient::isClient to return true - semantically they are managed so I think it's nicer - but I'm ok with this

I don't see why we shouldn't set ShellClient::isClient to return true - semantically they are managed so I think it's nicer - but I'm ok with this

Iirc we have places where a static_cast is performed based on the result of isClient

This revision was automatically updated to reflect the committed changes.