[wayland] Rework xdg-shell implementation
AbandonedPublic

Authored by zzag on Mar 5 2020, 8:51 AM.

Details

Summary

This change splits the XdgShellClient class to better match existing
abstractions in the xdg-shell protocol and fix a few issues related to
sending configure events.

In the new client classes, configure events are handled differently.
Instead of blocking configure events, we try to send them as late as
possible. Delaying configure events will let us merge changeMaximize()
for X11 clients and Wayland clients and it also fixes the bug where
we don't send the final configure event when user has finished resizing
a window.

Given that configure events are not sent immediately, XdgSurfaceClient
keeps the last requested frame geometry and the last requested client
geometry

Test Plan

Tests pass.

Diff Detail

Repository
R108 KWin
Branch
split-xdg-shell-client-2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26882
Build 26900: arc lint + arc unit
zzag created this revision.Mar 5 2020, 8:51 AM
Restricted Application added a project: KWin. · View Herald TranscriptMar 5 2020, 8:51 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Mar 5 2020, 8:51 AM
zzag added a comment.Mar 5 2020, 8:52 AM

You need D27860, D27828, and D27859 to test this patch.

zzag added a comment.Mar 5 2020, 8:53 AM

I also would really love to hear feedback on the chosen design.

We should be discussing big changes before doing the full implementation. It's quite frustrating.

RE: Kwayland
Yes there are definitely some problems and it needs some directional planning and changes. That needs a broader mailing list discussion, it shouldn't be done within the context of any specific patch.
You've been promising to send an email on that for weeks now :(


The new wrappers weren't implemented in
KWayland because I cannot guarantee that the future versions of the
wrappers will be API compatible.

Whilst I do agree with the overall comments on kwayland, this specific case is a bad example.

WMBase is finally stable API. Any new changes within WMBase versions have to be backwards compatible at a protocol level. Those have proved to be relatively easy to maintain compatibility at a library level.
XdgShellV6 is obviously also stable at this point.

We know that XdgShellV6(v1) will /always/ be basically identical to XdgWmBase(v1). As XdgWmBase goes through different versions, we will be keeping an abstraction layer inside XdgShellClient back to v1.
It seems very backwards that we've successfully fought an abstraction layer over something unstable, then removed it the instant things are actually stable!


Ending on some positive things:

  • I like the split of XdgTopLevel and XdgPopup that 100% makes sense. Also paves the way for future roles nicely.
  • We probably do want to redo our interface side to be based around WMBase rather than XdgShellV5. A rewrite probably was needed.
  • I like use of Qt's wayland scanner. It is a lot easier to read.
autotests/integration/xdgshellclient_test.cpp
1319

This needs explanation.

The one thing that makes such a big change somewhat safe is the extensive unit tests.
If we change them at the same time, then we've lost all that.

zzag planned changes to this revision.Mar 5 2020, 12:23 PM

We should be discussing big changes before doing the full implementation. It's quite frustrating.

RE: Kwayland
Yes there are definitely some problems and it needs some directional planning and changes. That needs a broader mailing list discussion, it shouldn't be done within the context of any specific patch.
You've been promising to send an email on that for weeks now :(

I've sent an email to a couple of mailing lists. Let's discuss what we should about KWayland and then move back to this patch.

zzag added inline comments.Mar 5 2020, 12:26 PM
autotests/integration/xdgshellclient_test.cpp
1319

Urgh, I made a mistake in the window geometry patch series.

The current window geometry is QRect(10, 10, 180, 80) and the current bounding rect is QRect(0, 0, 100, 50). Their intersection produces QRect(10, 10, 90, 40).

univerz added a subscriber: univerz.Mar 6 2020, 3:21 AM

Throwing out an alternate obvious solution for the code duplication; can we just drop xdgshellv6?
No point having a tonne of work for only hypothetical cases.

Few distros will have old GTK and a brand new Plasma. Could you check if anyone would be affected?

zzag added a comment.Mar 16 2020, 10:00 AM

Throwing out an alternate obvious solution for the code duplication; can we just drop xdgshellv6?
No point having a tonne of work for only hypothetical cases.

I wouldn't mind dropping support for xdg-shell-unstable-v6 because xdg_wm_base has been around for quite a while. However, I don't consider code duplication to be a huge problem right now. It's bad, but we have more serious problems.

Few distros will have old GTK and a brand new Plasma. Could you check if anyone would be affected?

Sure, I can do it.

zzag added a comment.Mar 16 2020, 10:52 AM
Toolkitxdg-shell v6xdg-shell stable
GTK3.21.53.22.30
Qt5.105.12

The table above shows when GTK and Qt gained support for xdg-shell v6 and xdg-shell stable.

Distribution (Release)GTKQt
Ubuntu (18.04)3.22.305.9.5
Ubuntu (19.10)3.24.125.12.4
Debian (Buster)3.24.55.11.3
Debian (Sid)3.24.145.12.5
openSUSE (Tumbleweed 03/16/2020)3.24.145.14.1
openSUSE (Leap 15.1)3.22.305.9.7
openSUSE (Leap 15.2)3.24.145.12.7
Arch Linux (03/16/2020)3.24.145.14.1
Fedora (Rawhide)3.24.145.13.2
Fedora (31)3.24.135.13.2

The table above shows GTK and Qt being shipped by major distributions.

Given that KDE Plasma 5.19+ is going to depend on Qt 5.14+, I think we can safely drop support for xdg-shell v6.

Seems like a no-brainer then.

It means this patch tells a nicer story. We're getting rid of all abstraction now there's one stable thing and we can rely on everything using that.

There's a bit to follow up on the ML wrt the protocol location. Then we can do start a code review.

autotests/integration/xdgshellclient_test.cpp
1319

why is it only coming up now?

The change in placement.cpp?

Can they be standalone?

xdgshellv6interface.cpp
736 ↗(On Diff #76998)

Not luck, I was the one who changed WMBase for that very reason, glad you agree \o/

zzag added a comment.Mar 17 2020, 11:58 AM

There's a bit to follow up on the ML wrt the protocol location. Then we can do start a code review.

Yep, sounds good. :)

autotests/integration/xdgshellclient_test.cpp
1319

Urgh, I made a mistake in the window geometry patch series.

Actually, I did not make a mistake.

Originally, the window geometry patch series assumed that we have to compute the effective window geometry when the pending state of xdg_surface is applied.

Then D26233, changed when the window geometry is clamped. It's worth to point out that we didn't take into account the frame geometry in D26233. So, it's quite possible that the frame geometry and the client size may diverge from time to time.

Given that XdgSurfaceClient clamps the window geometry when the main surface has been committed, the frame geometry and the client size cannot diverge anymore. So, this test must be adjusted.

xdgshellv6interface.cpp
736 ↗(On Diff #76998)

Thank you. :-)

zzag updated this revision to Diff 77977.Mar 19 2020, 7:16 AM
  • Rebase on master
  • Drop xdg-shell-v6

Because KWayland is a KF library,
we are not able to change existing API in ways that may break API or
ABI compatibility. So, if some particular wrapper has been implemented
incorrectly, we can't do that much about it except maybe leaving
comments that look like "// TODO KF6".

My 2 cents on this, that's normal implementation dependant situation in library, so about me it has 2 approaches:

  1. Always adds new required implementation in new class, then port existing users on it, leave orphaned classes to be removed
  2. To not break API/ABI it can be added a constructor by dependency injection the new implementation. It's a bit tricky, KTextEditor uses it, but it's not that bad.

Returning implementation to the client can be good if it's not expected any other client(s) to use it, if not that's not good.

zzag updated this revision to Diff 82070.May 6 2020, 9:01 AM
zzag edited the summary of this revision. (Show Details)

Rebase on KWaylandServer changes.

zzag updated this revision to Diff 82186.May 7 2020, 10:07 AM

Do not discard configure events in XdgSurfaceClient::sendConfigure()

zzag updated this revision to Diff 82644.Tue, May 12, 9:19 AM

Destroy XdgToplevelClient and XdgPopupClient on unmap.

Sorry for such an intrusive patch series. I hope that reviewing it
will be much easier on GitLab.

zzag edited the summary of this revision. (Show Details)Tue, May 12, 9:20 AM

Generally very nice and tidy. Will be even more so once we remove PlasmaShellsurface.

I have some minor comments.

I probably need to give it a second pass as it was quite overwhelming. Remember to drop the "WIP:" when ready

wayland_server.cpp
149–152

This can never be true.
It's a protocol error on XDGShell to map before configure

waylandxdgshellintegration.cpp
82

This method seems rather pointless. We are in specialisations for registerXdgTopLevel / popup then we call a generic method just to switch on whether it's a popup or a toplevel.

xdgshellclient.cpp
51

We leak the m_lastAckedConfigure if someone calls ackConfigure and gets deleted before commit.

I'm guessing you didn't use unique_ptr because of the QQueue, but storing via a qsharedpointer would work nicely.

1483

I can't see panelTakesFocusChanged.

I strongly suspect this will break Kai's reply notifications.

Arguably the event name doesn't match the usage, but this isn't the time and place for that.

zzag added inline comments.Tue, May 12, 11:31 AM
wayland_server.cpp
149–152

Well, yeah. I just wanted to keep things as generic as possible. Will remove it.

xdgshellclient.cpp
1483

There's a good chance that I overlooked it while I was resolving merge conflicts.

zzag updated this revision to Diff 82652.Tue, May 12, 12:21 PM
zzag marked 3 inline comments as done.

Address d_ed's inline comments.

zzag added inline comments.Tue, May 12, 12:22 PM
wayland_server.cpp
149–152

Hmm, apol has a patch series that implements zwp_input_panel_surface_v1. Unfortunately, the input method spec allows clients to create input panel surfaces with already attached buffers. Given that we'll need this code path later, I suggest to keep it so we don't have to re-introduce it again.

xdgshellclient.cpp
51

Yes, I didn't want to use QScopedPointer because it's not a movable type. On the other hand, I considered QSharedPointer to be an overkill so I decided to do memory management the hard way. Anyway, I think we could use QScopedPointer for storing the last acknowledged configure event.

zzag marked 5 inline comments as done.Tue, May 12, 12:22 PM
zzag updated this revision to Diff 82707.Wed, May 13, 5:58 AM

Some refactoring.

zzag updated this revision to Diff 82810.Thu, May 14, 6:03 AM
zzag retitled this revision from WIP: Rework xdg-shell implementation to [wayland] Rework xdg-shell implementation.

Update.

zzag abandoned this revision.Tue, May 19, 7:38 AM

Will move the code review to GitLab.