D6591: XdgV6 - Kwin side
This revision was automatically updated to reflect the committed changes. Closed by commit R108:e492f9e2980a: XdgV6 - Kwin side (authored by davidedmundson). REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=19895=19899 REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES autotests/integration/decoration_input_test.cpp autotests/integration/dont_crash_no_border.cpp autotests/integration/effects/fade_test.cpp autotests/integration/helper/kill.cpp autotests/integration/kwin_wayland_test.h autotests/integration/move_resize_window_test.cpp autotests/integration/plasma_surface_test.cpp autotests/integration/pointer_constraints_test.cpp autotests/integration/scene_qpainter_test.cpp autotests/integration/shell_client_test.cpp autotests/integration/test_helpers.cpp shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
graesslin accepted this revision. graesslin added a comment. This revision is now accepted and ready to land. And sorry, sorry, sorry that the review took so long. REPOSITORY R108 KWin BRANCH xdgv6 REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
davidedmundson updated this revision to Diff 19895. davidedmundson added a comment. Restricted Application edited projects, added Plasma; removed KWin. - Update tests to cover XDGv6 - write a ping test - fix crash in ping with socket mode connections REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=19394=19895 BRANCH xdgv6 REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES autotests/integration/decoration_input_test.cpp autotests/integration/dont_crash_no_border.cpp autotests/integration/effects/fade_test.cpp autotests/integration/helper/kill.cpp autotests/integration/kwin_wayland_test.h autotests/integration/move_resize_window_test.cpp autotests/integration/plasma_surface_test.cpp autotests/integration/pointer_constraints_test.cpp autotests/integration/scene_qpainter_test.cpp autotests/integration/shell_client_test.cpp autotests/integration/test_helpers.cpp shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
davidedmundson commandeered this revision. davidedmundson edited reviewers, added: mart; removed: davidedmundson. Restricted Application edited projects, added KWin; removed Plasma. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
davidedmundson added a comment. > It's really not difficult Ha. Extending those tests was not dificult, and if you look you'll see that was committed in the XDG branch mid last week. But we also have ping mixed in here (something I now regret), and that proved much more "interesting". Socket mode would have a fun crash as killWindow() will implicitly delete "this", also in the process I made a made the kill test represent a far more real blocked app, and that broke the existing killWindow socket mode test, as removing the connection doesn't actually kill a frozen app. All fixed now. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
graesslin added a comment. I really would like to see this go in now into master! But we need to have at least a few unit tests. If that's too difficult I can write them, but I would prefer if one of you extends it. It's really not difficult in this case as we just need to add the basics to run the same tests also as xdg shell v6. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
graesslin requested changes to this revision. graesslin added a comment. This revision now requires changes to proceed. Now it looks good to me! What I would like to see is unit tests for all of that. This should be fairly simple as the tests are prepared to be run for different kind of shell surfaces. Many tests already have a _data method where it is set to be run for wl_shell and xdg_shell_unstable_v5. This should be easy to be extended for additional v6. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
mart updated this revision to Diff 19394. mart added a comment. Restricted Application edited projects, added Plasma; removed KWin. - Redo popup grab handling - don't ping a window before killing setActive works also for wl_shell - move Ping Reason in own class REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=19373=19394 BRANCH xdgv6 REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
mart updated this revision to Diff 19372. mart added a comment. Restricted Application edited projects, added Plasma; removed KWin. - Temporary popup stuff - Update to signal rename - skip taskbar/pager/windowmanagement for xdgpopups - send configure on popup resize - Merge branch 'mart/xdgv6ping' into xdgv6 - Connect XDG ping pong only in XDG specific code path - Fixup - Merge branch 'master' into xdgv6 - move other xdg-only connect - Guard against assuming we're using xdgshell - disconnect ping timeout on shellclient destruction - Merge branch 'master' into xdgv6 - ping changes - don't do 3 casts - don't ping a window before killing REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=19310=19372 BRANCH xdgv6 REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
mart commandeered this revision. mart edited reviewers, added: davidedmundson; removed: mart. Restricted Application edited projects, added KWin; removed Plasma. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: mart, #plasma, graesslin, davidedmundson Cc: mart, graesslin, kwin, plasma-devel, #kwin, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
mart added inline comments. INLINE COMMENTS > graesslin wrote in shell_client.cpp:608-609 > That logic I don't understand: why do we ping to close? Why a roundtrip to > the app, when all we want to do is close it? Just send it a close? i copied from the x11 client, where on client::CloseWindow it pings beforehand, but i can remove it and just close REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> mart wrote in shell_client.cpp:608-609
> the close is actually beaing done at line 244: at the pongReceived (if the
> ping reason was closewindow, then it closes it there)
That logic I don't understand: why do we ping to close? Why a roundtrip to the
app, when all we want to do is close it? Just send it a close?
> shell_client.cpp:894-898
> +if (m_xdgShellSurface && rules()->checkAcceptFocus(wantsInput())) {
> +const qint32 pingSerial = static_cast *>(m_xdgShellSurface->global())->ping(m_xdgShellSurface);
> +m_pingSerials.insert(pingSerial, FocusWindow);
> setActive(true);
> }
This breaks the existing code: the setActive(true) is now only done for xdg
shell surfaces, not for wl_shell surfaces.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D6591
To: davidedmundson, #plasma, graesslin, mart
Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
davidedmundson updated this revision to Diff 19310. davidedmundson added a comment. Restricted Application edited projects, added Plasma; removed KWin. popup changes REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=19302=19310 BRANCH asdf REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
davidedmundson commandeered this revision. davidedmundson edited reviewers, added: mart; removed: davidedmundson. davidedmundson added inline comments. Restricted Application edited projects, added KWin; removed Plasma. INLINE COMMENTS > graesslin wrote in shell_client.cpp:277-280 > this needs to go through our PointerInputRedirection code, otherwise > SeatInterface and PointerInputRedirection get out of sync. From my > understanding that should be ::popupGrab? Ack, Initially I thought this event could happen at any time, but on re-reading it's to separate tooltips and menus, at which point it's on of the initial properties that should be sent before the first buffer. (I'll try to get that clarified in the spec for v7) At which point it falls in nicely into how kwin already handles things checking hasPopupGrab when the toplevel is added. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma, graesslin, mart Cc: mart, graesslin, kwin, plasma-devel, #kwin, bwowk, ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol
D6591: XdgV6 - Kwin side
graesslin requested changes to this revision.
graesslin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> shell_client.cpp:222
> +
> +connect(static_cast *>(m_xdgShellSurface->global()), ::pingDelayed,
> +m_xdgShellSurface, [this](qint32 serial) {
maybe instead of the three casts just store a local variable?
auto global = (static_cast(m_xdgShellSurface->global());
> shell_client.cpp:231-239
> +connect(static_cast *>(m_xdgShellSurface->global()), ::pingTimeout,
> +m_xdgShellSurface, [this](qint32 serial) {
> +auto it = m_pingSerials.find(serial);
> +if (it != m_pingSerials.end()) {
> +qCDebug(KWIN_CORE) << "Final ping timeout, asking to
> kill:" << caption();
> +killWindow();
> +m_pingSerials.erase(it);
You should only kill if the ping reason is close. This would also ping for
focus, wouldn't it?
> shell_client.cpp:277-280
> +connect(m_xdgShellPopup, ::grabbed, this,
> [this](SeatInterface *seat, quint32 serial) {
> +Q_UNUSED(serial)
> +seat->setFocusedPointerSurface(surface());
> +});
this needs to go through our PointerInputRedirection code, otherwise
SeatInterface and PointerInputRedirection get out of sync. From my
understanding that should be ::popupGrab?
> shell_client.cpp:608-609
> if (m_xdgShellSurface && isCloseable()) {
> -m_xdgShellSurface->close();
> -return;
> -}
> -if (m_qtExtendedSurface && isCloseable()) {
> +const qint32 pingSerial = static_cast *>(m_xdgShellSurface->global())->ping();
> +m_pingSerials.insert(pingSerial, CloseWindow);
> +} else if (m_qtExtendedSurface && isCloseable()) {
just wondering: isn't there a close() missing? It's only pinging...
> shell_client.h:44
> public:
> +enum PingReason {
> +CloseWindow = 0,
please use enum class for new enums.
REPOSITORY
R108 KWin
REVISION DETAIL
https://phabricator.kde.org/D6591
To: davidedmundson, #plasma, graesslin
Cc: graesslin, kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D6591: XdgV6 - Kwin side
davidedmundson added a comment. Ping. Even if I now can't merge till 5.11 splits, it'd be nice to have this approved. There's still a lot of work to be done on top of this, which is currently blocked. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma Cc: kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D6591: XdgV6 - Kwin side
davidedmundson updated this revision to Diff 16414. davidedmundson added a comment. Restricted Application edited projects, added Plasma; removed KWin. Lesson learned, don't try and use phab whilst rebasing it just explodes... REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=16413=16414 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES shell_client.cpp shell_client.h wayland_server.cpp wayland_server.h To: davidedmundson, #plasma Cc: kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
D6591: XdgV6 - Kwin side
davidedmundson updated this revision to Diff 16413. davidedmundson edited the summary of this revision. davidedmundson added a comment. Restricted Application edited projects, added KWin; removed Plasma. Fix whitespace REPOSITORY R108 KWin CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6591?vs=16410=16413 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6591 AFFECTED FILES shell_client.cpp To: davidedmundson, #plasma Cc: kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart, lukas
D6591: XdgV6 - Kwin side
davidedmundson retitled this revision from "XdgV6" to "XdgV6 - Kwin side". davidedmundson edited the summary of this revision. davidedmundson edited the test plan for this revision. REPOSITORY R108 KWin REVISION DETAIL https://phabricator.kde.org/D6591 To: davidedmundson, #plasma Cc: kwin, plasma-devel, #kwin, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas
