Details

Reviewers

Group Reviewers

Commits

Summary

To move forward faster I thought we could start reviewing the davidedmundson/xdgv6 branch for the top level
whilst I continue the Popup stuff in a new branch.

Commits are made by both me and Marco.
He did pings + min/max sizes I did most most the rest.

The main clever part that's not just boring boiler plate is how we handle the structure change
A surface now has an XDGSurface which then has a an Xdg TopLevel or a Xdg Popup

We need to fit this into the public API which assumes a surface has a Surface or a Popup.
The old Surface is similar to the new TopLevel.

The shoehorning works by relying on the fact that a surface without a role is pretty useless.

Clients create the surface implicitly with the toplevel or implicitly with the popup.
The server only announced it has a new "XdgSurface" when it gets a new zxdg_surface_get_toplevel.

Popup decisions:

On popup creation the server should copy the current info from the positioner and then it gets deleted. Given kwaylands job is to keep state, we expose all these parameter via popup.
Due to this positioner is not exposed as a resource anywhere.
Server API is 100% backwards compatiable.

i.e new code will work identically with v5 clients.

Client API is not. Grabs are called separately from the constructor, and the parent surface changed to an xdgsurface, not a raw surface.

V5 code still works as-is, just not with the new constructors.

It seemed better to match the v6 (and what will be the stable v7) than to try and do hacks and lose functionality.
Given the client needs to change the code to opt into V6 anyway. I don't think this is a huge problem.

Test Plan

Current test still passes.

Diff Detail

Repository

R127 KWayland

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

davidedmundson created this revision.May 31 2017, 10:16 AM

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptMay 31 2017, 10:16 AM

Flipping phabricator...

Arc didn't work, I think it was too big.
Manually uploading the patch did.

Comments for Marco:

src/server/xdgshell_interface.h
80	We need the serial Ids here, otherwise it's not very usable; especially as the pong doesn't have an elapsed time. A kjob like API wrapping this might be perfect for here?
src/server/xdgshell_interface_p.h
41	I don't understand this timer, all it's used for is for making us not emit a pong if it comes in after a timeout?

mart added a subscriber: mart.Jun 1 2017, 3:34 PM

mart added inline comments.

src/server/xdgshell_interface.h
80	so storing all the ids of pings in progress somewhere to make a more recent ping not cancel an older one still pending?
src/server/xdgshell_interface_p.h
41	send ping, if a pong doesn't arrive after a timeout, consider it dead.. I copied it as-is from wl_shell iirc, so assumed this would be the expected behavior.

mart added inline comments.Jun 5 2017, 11:25 AM

src/server/xdgshell_interface.h
80	from kwin, client.cpp: i guess that's the kwin part that will have to use this. it uses a single timer, and tries for a couple of timeout, as soon as the client answers it's considered good, no serial for pings is considered. void Client::pingWindow() { if (!info->supportsProtocol(NET::PingProtocol)) return; // Can't ping :( if (options->killPingTimeout() == 0) return; // Turned off if (ping_timer != NULL) return; // Pinging already ping_timer = new QTimer(this); connect(ping_timer, &QTimer::timeout, this, [this]() { if (unresponsive()) { qCDebug(KWIN_CORE) << "Final ping timeout, asking to kill:" << caption(); ping_timer->deleteLater(); ping_timer = nullptr; killProcess(true, m_pingTimestamp); return; } qCDebug(KWIN_CORE) << "First ping timeout:" << caption(); setUnresponsive(true); ping_timer->start(); } );

src/server/xdgshell_interface.h

from kwin, client.cpp:
i guess that's the kwin part that will have to use this.
it uses a single timer, and tries for a couple of timeout, as soon as the client answers it's considered good, no serial for pings is considered.

void Client::pingWindow()
{

if (!info->supportsProtocol(NET::PingProtocol))
    return; // Can't ping :(
if (options->killPingTimeout() == 0)
    return; // Turned off
if (ping_timer != NULL)
    return; // Pinging already
ping_timer = new QTimer(this);
connect(ping_timer, &QTimer::timeout, this,
    [this]() {
        if (unresponsive()) {
            qCDebug(KWIN_CORE) << "Final ping timeout, asking to kill:" << caption();
            ping_timer->deleteLater();
            ping_timer = nullptr;
            killProcess(true, m_pingTimestamp);
            return;
        }

        qCDebug(KWIN_CORE) << "First ping timeout:" << caption();

        setUnresponsive(true);
        ping_timer->start();
    }
);

davidedmundson updated this revision to Diff 15634.Jun 20 2017, 9:55 AM

davidedmundson edited the summary of this revision. (Show Details)

could you please upload a version with context?

src/server/xdgshell_interface.h
75	what is "gravity"?
80	please note that this is X11 code which has a different ping concept. On Wayland we don't ping yet.
80	@davidedmundson for the ping you could check the old wl_shell_surface implementation in KWayland.
124	please add what the replacement is
142	can we have a better name than popupCreated2?
145	documentation missing

mart added inline comments.Jun 23 2017, 11:29 AM

src/server/xdgshell_interface.h
75	as far i understood, it's an hint that tells the window the direction it prefers to popup, so like "please dear compositor make it show below the point i asked to", or at the left, or whatever hint that the compositor can choose to ignore
80	the implementation here in xdgshell_interface is basically copied over the kwayland wl_shell_surface. the code pasted above, is the only kwin part i found that was managing pings in any way, which is the x11 implementation (Client) i guess ShellClient will have to have something along the lines using the kwayland implementations, tough casing between the possible interfaces exposed (wl_shell, xdgshellv5 and xdgshellv6)

Try to upload via arc...

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptJun 23 2017, 4:59 PM

trying to implement the kwin part of ping btw, will do in a branch of the branch kwin/xdgv6

cleanups

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptJun 26 2017, 5:02 PM

Done all comments except ping/pong.
Marco, ping me if you ever want this updating.

In D6047#119698, @mart wrote:

trying to implement the kwin part of ping btw, will do in a branch of the branch kwin/xdgv6

no need to hurry with it. IMHO the way ping works is completely broken and won't work. At least Qt holds the connection in a thread, so it will react to the ping even if the application is frozen in the main gui thread.

Also please note that ping needs to be different in Wayland compared to X11. In X11 we only ping when trying to close the window. In Wayland we should ping whenever we interact with the window in some way. E.g. when we pass pointer focus, when we pass keyboard focus, maybe even when sending in input events.

In D6047#119765, @graesslin wrote:

In D6047#119698, @mart wrote:

trying to implement the kwin part of ping btw, will do in a branch of the branch kwin/xdgv6

no need to hurry with it. IMHO the way ping works is completely broken and won't work. At least Qt holds the connection in a thread, so it will react to the ping even if the application is frozen in the main gui thread.

any way to change this/make the gui thread actually answer?

Also please note that ping needs to be different in Wayland compared to X11. In X11 we only ping when trying to close the window. In Wayland we should ping whenever we interact with the window in some way. E.g. when we pass pointer focus, when we pass keyboard focus, maybe even when sending in input events.

if it's done in all input events, couldn't this affect performance?

right now, the xdgv6 branch (xdgv6popup actually) for me causes a crash in kwin at startup, with the following BT:
#0 _int_malloc (av=av@entry=0x7ffff556ab20 <main_arena>, bytes=bytes@entry=72) at malloc.c:3414
#1 0x00007ffff522a5d4 in GI_libc_malloc (bytes=72) at malloc.c:2911
#2 0x00007ffff55fde78 in operator new(unsigned long) ()

from /usr/lib/x86_64-linux-gnu/libstdc++.so.6

#3 0x00007ffff5dcc32e in QObjectPrivate::connectImpl (sender=sender@entry=0x69d950,

signal_index=5, receiver=receiver@entry=0x69c600, slot=slot@entry=0x0, 
slotObj=slotObj@entry=0x69c860, type=Qt::AutoConnection, types=0x0, 
senderMetaObject=0x7ffff720c220 <KWayland::Server::Display::staticMetaObject>)
at kernel/qobject.cpp:4829

#4 0x00007ffff5dcc69b in QObject::connectImpl (sender=0x69d950, signal=<optimized out>,

receiver=0x69c600, slot=0x0, slotObj=0x69c860, type=Qt::AutoConnection, types=0x0, 
senderMetaObject=0x7ffff720c220 <KWayland::Server::Display::staticMetaObject>)
at kernel/qobject.cpp:4784

#5 0x00007ffff6f1f249 in QObject::connect<void (KWayland::Server::Display::*)(), KWayland::Server::Display::createXdgShell(const KWayland::Server::XdgShellInterfaceVersion&, QObject*)::<lambda()> >(const QtPrivate::FunctionPointer<void (KWayland::Server::Display::*)()>::Object *, void (KWayland::Server::Display::*)(KWayland::Server::Display * const), const QObject *, KWayland::Server::Display::<lambda()>, Qt::ConnectionType) (sender=0x69d950,

signal=(void (KWayland::Server::Display::*)(KWayland::Server::Display * const)) 0x7ffff6fa67de <KWayland::Server::Display::aboutToTerminate()>, context=0x69c600, slot=..., 
type=Qt::AutoConnection) at /opt/kde5/include/QtCore/qobject.h:338

Update from IRC chat:
It was BIC from when I renamed that signal. All resolved.

davidedmundson retitled this revision from WIP: Support XDG v6 to Support XDG v6.Jun 29 2017, 3:48 PM

Unbind global
Add a test for transientFor

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptAug 7 2017, 8:59 AM

First round of review, client api is done.

src/client/registry.h
1275	careful here: the since won't match
src/client/xdgshell.h
95	In case you wanted to make those comments doxygen comments: you need to use /** instead of /* Applies for all comments in this class.
176	lacking documentation including an @since.
222	If the comment is deprecated, the method should be deprecated as well. But I disagree on the point of deprecation. It is not deprecated if it's used with xdgShellv5. That should probably just be documented properly.
226	@since missing
229	looks like wrong indentation.
231	@since missing
302	missing documentation.
427	same as with the other class: that's not a documentation comment.
500	@since missing
562	documentation missing
src/client/xdgshell_p.h
67	Please no qWarning. Either qCWarning or not at all.
src/client/xdgshell_v6.cpp
28	I don't see any debug messages in this file

This revision now requires changes to proceed.Aug 25 2017, 2:05 PM

added some comments

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptAug 26 2017, 11:44 AM

graesslin added inline comments.Sep 3 2017, 8:08 AM

src/server/xdgshell_interface.cpp
43 ↗	(On Diff #15889)	what's the idea behind a mutable lambda?
src/server/xdgshell_interface.h
438 ↗	(On Diff #15889)	If it's something the client request, I would suggest a rename to grabRequested. Could you please explain what "grab" is supposed to be?

davidedmundson marked an inline comment as done.Sep 3 2017, 12:45 PM

davidedmundson added inline comments.

src/server/xdgshell_interface.cpp
43 ↗	(On Diff #15889)	the captured "attempt" variable is modified inside the lambda

Rename grab signal

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptSep 3 2017, 8:35 PM

mart added inline comments.Sep 4 2017, 8:45 AM

src/server/xdgshell_interface.cpp
43 ↗	(On Diff #15889)	needs to tack the state as needs to track that only a couple of attempts are made

graesslin accepted this revision.Sep 4 2017, 2:58 PM

This revision is now accepted and ready to land.Sep 4 2017, 2:58 PM

Closed by commit R127:d4c82b60cf16: Support XDG v6 (authored by davidedmundson). · Explain WhySep 4 2017, 4:02 PM

This revision was automatically updated to reflect the committed changes.

davidedmundson marked an inline comment as done.

		Path
M		autotests/client/CMakeLists.txt (13 lines)
A	M	autotests/client/test_xdg_shell.h (101 lines)
M		autotests/client/test_xdg_shell.cpp (169 lines)
A	M	autotests/client/test_xdg_shell_v5.cpp (72 lines)
A	M	autotests/client/test_xdg_shell_v6.cpp (201 lines)
M		src/client/CMakeLists.txt (5 lines)
A	M	src/client/protocols/xdg-shell-unstable-v6.xml (1044 lines)
M		src/client/registry.h (28 lines)
M		src/client/registry.cpp (11 lines)
M		src/client/xdgshell.h (168 lines)
M		src/client/xdgshell.cpp (160 lines)
M		src/client/xdgshell_p.h (153 lines)
M		src/client/xdgshell_v5.cpp (28 lines)
A	M	src/client/xdgshell_v6.cpp (585 lines)
M		src/server/CMakeLists.txt (6 lines)
M		src/server/display.cpp (4 lines)
M		src/server/xdgshell_interface.h (174 lines)
M		src/server/xdgshell_interface.cpp (98 lines)
M		src/server/xdgshell_interface_p.h (28 lines)
M		src/server/xdgshell_v5_interface.cpp (38 lines)
M		src/server/xdgshell_v5_interface_p.h (2 lines)
A	M	src/server/xdgshell_v6_interface.cpp (962 lines)
A	M	src/server/xdgshell_v6_interface_p.h (148 lines)
M		src/tools/mapping.txt (3 lines)
M		tests/CMakeLists.txt (5 lines)
A	M	tests/xdgtest.cpp (176 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	15013	877e953		May 31 2017, 10:15 AM	★	★
Diff 2	15020			May 31 2017, 1:02 PM	★	★
Diff 3	15634			Jun 20 2017, 9:46 AM	★	★
Diff 4	15797	faedcd4	Try to upload via arc...	Jun 23 2017, 4:59 PM	★	★
Diff 5	15889	faedcd4	- cleanups	Jun 26 2017, 5:02 PM	★	★
Diff 6	17813	73cdeb9	Unbind global	Aug 7 2017, 8:59 AM	★	★
Diff 7	18807	16f4486	added some comments	Aug 26 2017, 11:44 AM	★	★
Diff 8	19148	670e831	Rename grab signal	Sep 3 2017, 8:35 PM	★	★
Diff 9	19169	670e831	R127:d4c82b60cf161555dbd7875841ce555b1f99b37c	Sep 4 2017, 3:56 PM	★	★

Support XDG v6
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 17813

autotests/client/CMakeLists.txt

autotests/client/test_xdg_shell.h

autotests/client/test_xdg_shell.cpp

autotests/client/test_xdg_shell_v5.cpp

autotests/client/test_xdg_shell_v6.cpp

src/client/CMakeLists.txt

src/client/protocols/xdg-shell-unstable-v6.xml

src/client/registry.h

src/client/registry.cpp

src/client/xdgshell.h

src/client/xdgshell.cpp

src/client/xdgshell_p.h

src/client/xdgshell_v5.cpp

src/client/xdgshell_v6.cpp

src/server/CMakeLists.txt

src/server/display.cpp

src/server/xdgshell_interface.h

src/server/xdgshell_interface.cpp

src/server/xdgshell_interface_p.h

src/server/xdgshell_v5_interface.cpp

src/server/xdgshell_v5_interface_p.h

src/server/xdgshell_v6_interface.cpp

src/server/xdgshell_v6_interface_p.h

src/tools/mapping.txt

tests/CMakeLists.txt

tests/xdgtest.cpp

Support XDG v6ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 17813

autotests/client/CMakeLists.txt

autotests/client/test_xdg_shell.h

autotests/client/test_xdg_shell.cpp

autotests/client/test_xdg_shell_v5.cpp

autotests/client/test_xdg_shell_v6.cpp

src/client/CMakeLists.txt

src/client/protocols/xdg-shell-unstable-v6.xml

src/client/registry.h

src/client/registry.cpp

src/client/xdgshell.h

src/client/xdgshell.cpp

src/client/xdgshell_p.h

src/client/xdgshell_v5.cpp

src/client/xdgshell_v6.cpp

src/server/CMakeLists.txt

src/server/display.cpp

src/server/xdgshell_interface.h

src/server/xdgshell_interface.cpp

src/server/xdgshell_interface_p.h

src/server/xdgshell_v5_interface.cpp

src/server/xdgshell_v5_interface_p.h

src/server/xdgshell_v6_interface.cpp

src/server/xdgshell_v6_interface_p.h

src/tools/mapping.txt

tests/CMakeLists.txt

tests/xdgtest.cpp

Support XDG v6
ClosedPublic
Actions

Revision Contents
Changeset List