Details

Reviewers

davidedmundson
graesslin

Group Reviewers

Plasma
KWin

Commits

R127:a742323a3027: wrap XdgForeignUnstableV1 to XdgForeignUnstable
R127:ce2ff96c5756: Wayland foreign protocol
R127:9fbeedfe1173: add a serverside setTransientFor in xdgshell
R127:d8bcc590bdf9: name the methods exportTopLevel/importTopLevel
R127:98a9315d8af4: remove Unstable from public symbols
R127:fee82ecdd8dc: unexport classes that should be private

Summary

Implement the "foreign" wayland protocol.
A client can export a surface with an unique string as handle,
then another client can refer to that surface and set an own surface as
child of that surface.
Potential use cases are out-of-process dialogs, such as file dialogs,
meant to be used by sandboxed processes that may not have the access
it needs to implement such dialogs.
The handle needs to be shared between the processes with other means,
such as dbus or command line paramenters.

The public api of the server side only tracks parent/child relationships as this is the only data kwin would need it for, the rest of the api is not exported so should be safer from eventual protocol changes

Test Plan

the autotest works, but has a lot of random crashes when deleting surfaces,
unfortunately backtraces don't tell much and the crashes never occur when running into valgrind
behavior may still be wrong, depending on how the protocol is supposed
to work if more clients try to set the same exported surface as parent

Diff Detail

Repository

R127 KWayland

Branch

mart/xdgforeign

Lint

No Linters Available

Unit

No Unit Test Coverage

mart created this revision.Aug 17 2017, 3:47 PM

Restricted Application added projects: Plasma on Wayland, Frameworks. · View Herald TranscriptAug 17 2017, 3:47 PM

Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript

mart edited the summary of this revision. (Show Details)Aug 17 2017, 3:53 PM

mart added inline comments.

src/server/xdgforeign_v1_interface.cpp
213 ↗	(On Diff #18767)	the children hash may have to become a multihash to permit more children for a single parent

graesslin added a subscriber: graesslin.Aug 17 2017, 4:08 PM

graesslin added inline comments.

src/client/registry.h
940–941	The general pattern I used for protocols which might get another revision is to create a "meta" class which hides the UnstableV1 mess. C.f. the PointerConstraints a line above.
src/server/xdgforeign_v1_interface.cpp
164 ↗	(On Diff #18767)	shouldn't we verify that the Uuid is unique? I just checked the docs and don't see a requirement that the created Uuid is unique.
src/server/xdgforeign_v1_interface.h
38 ↗	(On Diff #18294)	also on the server I tried to hide the unstable mess from the class names. The advantage is that the user does not have to change all it's code to support this. Example is the text input protocol.

davidedmundson requested changes to this revision.Aug 17 2017, 4:39 PM

davidedmundson added a subscriber: davidedmundson.

davidedmundson added inline comments.

src/client/xdgforeign_v1.cpp
190 ↗	(On Diff #18767)	Some idiot left a comment all over your code after fixing it. We don't do the thing I was saying about anymore.
src/server/xdgforeign_v1_interface.cpp
300 ↗	(On Diff #18767)	this should be connected to unbound.
308 ↗	(On Diff #18767)	I don't think this is right. Second half of this line: A surface may be exported multiple times, and each exported handle may be used to create a xdg_imported multiple times.
498 ↗	(On Diff #18767)	the super class destructor (Resource::Private::~Private) does this
src/server/xdgforeign_v1_interface.h
51 ↗	(On Diff #18294)	docs, especially on what either argument being null means.

This revision now requires changes to proceed.Aug 17 2017, 4:39 PM

mart added inline comments.Aug 18 2017, 10:19 AM

src/client/registry.h
940–941	i see the types are named zxdg_exporter_v1 because it's how the interface is calledin the wayland protocol xml. woudl i need to make another copy of the xml where the interface name is unversioned?

mart marked 2 inline comments as done.Aug 18 2017, 10:28 AM

mart added inline comments.

src/server/xdgforeign_v1_interface.cpp
300 ↗	(On Diff #18767)	perhaps the connects to the destruction of the SurfaceInterface* are not even necessary?

get rid of the v1 from the verver side
dress some issues
Q_DECL_HIDDEN for all non exported classes
track surface destruction
more attention to data integrity
wait for server surfaces deletion
more qpointers out of pure desperation

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptAug 18 2017, 4:07 PM

part of the comments adressed, still to wrap the client part in order to hide the v1 implementation.

the status right now is that, when running in valgrind all test pass, while then invoked directly it almost always crashes, but every time at a different, random place.
there is some serious memory corruption somewhere, but i'm completely at loss in figuring out where it is (and valgrind output, as gdb backtraces seem quite useless)

Don't call release twice
Correctly set
don't call destroy in the destroyed callback
add a test exporting a surface two times
test importing two times the same surface
wrap XdgForeignUnstableV1 to XdgForeignUnstable
unexport classes that should be private

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptAug 23 2017, 1:02 PM

now the client part is wrapped too, so no V1 exported (the public facing classes should lose "Unstable" probably)
two more tests to check that importing the same exported in two clients works and exporting the same surface two times works as well.
thanks to david's help most of the random crashes are fixed, there is still to solve one crash at exit of the autotest and the warning message:
wl_display@1: error 0: invalid object 9 in a couple of autotests, which still suggests something wrong.

i think it's getting near feature completness to implement the kwin part tough

add a manual test for foreign
remove Unstable from public symbols

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

add a serverside setTransientFor in xdgshell

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptAug 25 2017, 5:03 PM

get rid of setTransientFor

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptAug 25 2017, 5:47 PM

mart retitled this revision from [WIP] Wayland foreign protocol to Wayland foreign protocol.Sep 5 2017, 1:17 PM

Overall lots of documentation is missing.

From API layout that looks much better now, but we still have a few places where "Unstable" is needlessly in the API. Most important in Display the create method is not forward compatible by not having an enum to describe which interface version should be created.

src/client/registry.h
527–528 ↗	(On Diff #18767)	please add documentation
940–941 ↗	(On Diff #18767)	please add documentation
940–941 ↗	(On Diff #18767)	Why did you keep the suffix "Unstable"?
1132–1133 ↗	(On Diff #18767)	please add documentation
1288–1290 ↗	(On Diff #18767)	please add documentation
src/server/display.h
223 ↗	(On Diff #18767)	please add documentation. Also the common way would be to have an enum to describe which interface should be created, but therefore drop the Unstable from the API name.
src/server/xdgforeign_interface.h
37 ↗	(On Diff #18767)	Something like an enum ForeignInterfaceVersion is missing, please compare TextInputInterface
src/server/xdgforeign_v1_interface.cpp
260 ↗	(On Diff #18767)	what's an imp? Could you please write full variable names?

This revision now requires changes to proceed.Sep 5 2017, 4:57 PM

move to xdg-foreign-unstable-v2
get rid of setTransientFor

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptOct 2 2017, 3:55 PM

Documentation is still missing, Unstable suffix is still in two many API calls.

src/client/registry.h
527–528	documentation missing

This revision now requires changes to proceed.Oct 4 2017, 6:41 PM

In D7369#152358, @graesslin wrote:

Documentation is still missing, Unstable suffix is still in two many API calls.

i think the importerUnstableV2Announced exporterUnstableV2Announced should remain with unstable and versioned?

add api docs, getridof some unstable, calls with unstable follow the other interfaces

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptOct 10 2017, 11:11 AM

In D7369#153978, @mart wrote:

In D7369#152358, @graesslin wrote:

Documentation is still missing, Unstable suffix is still in two many API calls.

i think the importerUnstableV2Announced exporterUnstableV2Announced should remain with unstable and versioned?

Yes, those can stay. Other signals are similar.

update to the upstream xml protocol

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptOct 11 2017, 9:10 AM

In D7369#154153, @graesslin wrote:

In D7369#153978, @mart wrote:

In D7369#152358, @graesslin wrote:

Documentation is still missing, Unstable suffix is still in two many API calls.

i think the importerUnstableV2Announced exporterUnstableV2Announced should remain with unstable and versioned?

Yes, those can stay. Other signals are similar.

so how is the current state, can go in? (btw, the unstable-v2 protocol was just pushed to master, so another roadblock done)

There is still quite some documentation missing. Especially the server side is not yet documented enough that I would know how to use the new API and what it does. It's totally fine to copy and adapt the documentation from the Wayland xml protocol.

src/client/xdgforeign.h
117 ↗	(On Diff #18767)	Please add documentation
209 ↗	(On Diff #18767)	Please add documentation
269 ↗	(On Diff #18767)	what's the handle?
328 ↗	(On Diff #18767)	please add documentation
src/server/xdgforeign_interface.h
39 ↗	(On Diff #18767)	This needs documentation
46–47 ↗	(On Diff #18767)	Please add documentation? I wouldn't know what create is needed for.
50 ↗	(On Diff #18767)	did set "what"?

documentation++
name the methods exportTopLevel/importTopLevel

Restricted Application edited projects, added Plasma; removed Plasma on Wayland. · View Herald TranscriptOct 12 2017, 12:51 PM

more documentation

Restricted Application edited projects, added Plasma on Wayland; removed Plasma. · View Herald TranscriptOct 12 2017, 1:01 PM

graesslin accepted this revision.Oct 12 2017, 3:39 PM

Closed by commit R127:ce2ff96c5756: Wayland foreign protocol (authored by mart). · Explain WhyOct 13 2017, 9:32 AM

This revision was automatically updated to reflect the committed changes.

		Path
M		autotests/client/CMakeLists.txt (11 lines)
A	M	autotests/client/test_xdg_foreign.cpp (401 lines)
M		src/client/CMakeLists.txt (7 lines)
A	M	src/client/protocols/xdg-foreign-unstable-v1.xml (182 lines)
A	M	src/client/protocols/xdg-foreign-unstable-v2.xml (182 lines)
M		src/client/registry.h (22 lines)
M		src/client/registry.cpp (30 lines)
A	M	src/client/xdgforeign.h (350 lines)
A	M	src/client/xdgforeign.cpp (271 lines)
A	M	src/client/xdgforeign_p.h (113 lines)
A	M	src/client/xdgforeign_v1.h (153 lines)
A	M	src/client/xdgforeign_v1.cpp (337 lines)
A	M	src/client/xdgforeign_v2.h (153 lines)
A	M	src/client/xdgforeign_v2.cpp (337 lines)
M		src/server/CMakeLists.txt (8 lines)
M		src/server/display.h (2 lines)
M		src/server/display.cpp (8 lines)
A	M	src/server/xdgforeign_interface.h (76 lines)
A	M	src/server/xdgforeign_interface.cpp (79 lines)
A	M	src/server/xdgforeign_v1_interface.cpp (459 lines)
A	M	src/server/xdgforeign_v1_interface_p.h (124 lines)
A	M	src/server/xdgforeign_v2_interface.cpp (459 lines)
A	M	src/server/xdgforeign_v2_interface_p.h (124 lines)
M		tests/CMakeLists.txt (4 lines)
A	M	tests/xdgforeigntest.cpp (208 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	18294	c732c80		Aug 17 2017, 3:47 PM	★	★
Diff 2	18357	c732c80	- get rid of the v1 from the verver side	Aug 18 2017, 4:07 PM	★	★
Diff 3	18596	c732c80	- Don't call release twice	Aug 23 2017, 1:02 PM	★	★
Diff 4	18744	c732c80	- add a manual test for foreign	Aug 25 2017, 11:14 AM	★	★
Diff 5	18760	c732c80	- add a serverside setTransientFor in xdgshell	Aug 25 2017, 5:03 PM	★	★
Diff 6	18767	c732c80	- get rid of setTransientFor	Aug 25 2017, 5:47 PM	★	★
Diff 7	20257	c732c80	- move to xdg-foreign-unstable-v2	Oct 2 2017, 3:54 PM	★	★
Diff 8	20553	c732c80	- add api docs, getridof some unstable, calls with unstable follow the other…	Oct 10 2017, 11:11 AM	★	★
Diff 9	20584	c732c80	- update to the upstream xml protocol	Oct 11 2017, 9:10 AM	★	★
Diff 10	20638	c732c80	- documentation++	Oct 12 2017, 12:51 PM	★	★
Diff 11	20639	c732c80	- more documentation	Oct 12 2017, 1:00 PM	★	★
Diff 12	20667	daec657	R127:ce2ff96c5756b93847786cdd5e7dd58d80b83044	Oct 13 2017, 9:30 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
1597035022b4	001e66ed4c85	10cfa768faad	Marco Martin	get rid of setTransientFor	Aug 25 2017, 5:47 PM
10cfa768faad	2ffd3cbcc393	9fbeedfe1173	Marco Martin	move to xdg-foreign-unstable-v2 (Show More…)	Oct 2 2017, 3:07 PM
9fbeedfe1173	cd12cb6d2ac0	98a9315d8af4	Marco Martin	add a serverside setTransientFor in xdgshell	Aug 25 2017, 5:02 PM
98a9315d8af4	3b5b3b9e917a	c003bc07d8ad	Marco Martin	remove Unstable from public symbols	Aug 25 2017, 11:12 AM
c003bc07d8ad	86e4afd498ea	fee82ecdd8dc	Marco Martin	add a manual test for foreign	Aug 24 2017, 4:29 PM
fee82ecdd8dc	2b55fb766a9a	a742323a3027	Marco Martin	unexport classes that should be private	Aug 23 2017, 12:54 PM
a742323a3027	781286c0b16f	716aecc2250a	Marco Martin	wrap XdgForeignUnstableV1 to XdgForeignUnstable (Show More…)	Aug 23 2017, 12:36 PM
716aecc2250a	e925b002dd5e	7ed81e1bac26	Marco Martin	test importing two times the same surface	Aug 22 2017, 2:12 PM
7ed81e1bac26	a69ea8b4b299	824d52d5bb40	Marco Martin	add a test exporting a surface two times	Aug 22 2017, 2:09 PM
824d52d5bb40	08cfc4f1e1fb	1105f43014b9	David Edmundson	don't call destroy in the destroyed callback (Show More…)	Aug 21 2017, 2:57 PM
1105f43014b9	fba16532e9d1	770b4b0d5d0e	David Edmundson	Correctly set (Show More…)	Aug 21 2017, 2:52 PM
770b4b0d5d0e	381a36abc788	62901b9c00bf	David Edmundson	Don't call release twice (Show More…)	Aug 21 2017, 2:29 PM
62901b9c00bf	b0d00f4fe551	d14f0cf17da3	Marco Martin	more qpointers out of pure desperation	Aug 18 2017, 4:05 PM
d14f0cf17da3	2806aedc843c	e710cf7ae39e	Marco Martin	wait for server surfaces deletion	Aug 18 2017, 3:46 PM
e710cf7ae39e	c1f67a709b69	c1265aadb4a8	Marco Martin	more attention to data integrity	Aug 18 2017, 2:36 PM
c1265aadb4a8	7029a841f22c	2b3a03161d20	Marco Martin	track surface destruction	Aug 18 2017, 1:19 PM
2b3a03161d20	62882c2fb027	afb9e43dd0f0	Marco Martin	Q_DECL_HIDDEN for all non exported classes	Aug 18 2017, 10:46 AM
afb9e43dd0f0	ee9b6fa2085f	86805799fd5d	Marco Martin	dress some issues	Aug 18 2017, 10:25 AM
86805799fd5d	c27816b08219	d80e36ea9ac5	Marco Martin	get rid of the v1 from the verver side (Show More…)	Aug 18 2017, 10:03 AM
d80e36ea9ac5	7fac8fcc4afe	41fe0a430be8	Marco Martin	rename methods in import/export_toplevel	Aug 17 2017, 2:51 PM
41fe0a430be8	7a37714fa827	f40c47081fd0	Marco Martin	implement destroyedCallback	Aug 17 2017, 2:49 PM
f40c47081fd0	8d54bd213cf8	0a5b59596812	Marco Martin	move exported/imported interfaces in private header	Aug 17 2017, 2:12 PM
0a5b59596812	2917bf3179e5	f8b0de0b46e7	Marco Martin	test delete children and parent surfaces	Aug 17 2017, 1:42 PM
f8b0de0b46e7	53e495eafe21	2aa273b51932	Marco Martin	more tests	Aug 17 2017, 1:06 PM
2aa273b51932	8193ce5fb7f4	7835b5e8333c	Marco Martin	more tests	Aug 17 2017, 9:20 AM
7835b5e8333c	1362daff6fce	d42c900ab836	Marco Martin	start more tests	Aug 16 2017, 7:17 PM
d42c900ab836	c0ded00e7894	cbb5492829ed	Marco Martin	move importer and exporter in private header (Show More…)	Aug 16 2017, 4:28 PM
cbb5492829ed	aee9e127a3b7	c5cff9adcec4	Marco Martin	introduce a ForeignInterface class to wrap server side	Aug 16 2017, 4:22 PM
c5cff9adcec4	d2fccbe31c0a	a8d312198d0b	Marco Martin	test the proper serversurface has been imported	Aug 10 2017, 2:51 PM
a8d312198d0b	fa3833bd04bb	9e266f405d81	Marco Martin	more signal server side to be ableto test	Aug 10 2017, 2:41 PM
9e266f405d81	1e90f2625bd2	ce367ea1f41a	Marco Martin	make import actually work (Show More…)	Aug 10 2017, 12:27 PM
ce367ea1f41a	70fcacebcd7e	02282abb6d4c	Marco Martin	first rough implementation of import on server side	Aug 9 2017, 5:37 PM
02282abb6d4c	358b0b52872c	e4fc81679110	Marco Martin	implement basic exporting mechanics (Show More…)	Aug 9 2017, 2:24 PM
e4fc81679110	28bae3028fdf	5ac97e011d39	Marco Martin	more work in the autotest	Aug 8 2017, 4:24 PM
5ac97e011d39	ccb1c1d8f601	454d095d8518	Marco Martin	wire up the exporter/importer in registry (Show More…)	Aug 8 2017, 3:51 PM
454d095d8518	d58b816fceba	a669eccd7f96	Marco Martin	make the server part build (Show More…)	Aug 7 2017, 4:26 PM
a669eccd7f96	78271dcf78f1	4b0505766c4f	David Edmundson	Fix code	Jul 31 2017, 5:04 PM
4b0505766c4f	1b29bf30c79f	f3a7426826fe	Marco Martin	add early implementations of xdg-foreign (Show More…)	Jul 31 2017, 4:07 PM
f3a7426826fe	2af4cd99d22d	c732c8087cfe	Marco Martin	add the xdg foreign protocol definition	Jul 31 2017, 8:55 AM

Wayland foreign protocol
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 20257

autotests/client/CMakeLists.txt

autotests/client/test_xdg_foreign.cpp

src/client/CMakeLists.txt

src/client/protocols/xdg-foreign-unstable-v1.xml

src/client/protocols/xdg-foreign-unstable-v2.xml

src/client/registry.h

src/client/registry.cpp

src/client/xdgforeign.h

src/client/xdgforeign.cpp

src/client/xdgforeign_p.h

src/client/xdgforeign_v1.h

src/client/xdgforeign_v1.cpp

src/client/xdgforeign_v2.h

src/client/xdgforeign_v2.cpp

src/server/CMakeLists.txt

src/server/display.h

src/server/display.cpp

src/server/xdgforeign_interface.h

src/server/xdgforeign_interface.cpp

src/server/xdgforeign_v1_interface.cpp

src/server/xdgforeign_v1_interface_p.h

src/server/xdgforeign_v2_interface.cpp

src/server/xdgforeign_v2_interface_p.h

tests/CMakeLists.txt

tests/xdgforeigntest.cpp

Wayland foreign protocolClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 20257

autotests/client/CMakeLists.txt

autotests/client/test_xdg_foreign.cpp

src/client/CMakeLists.txt

src/client/protocols/xdg-foreign-unstable-v1.xml

src/client/protocols/xdg-foreign-unstable-v2.xml

src/client/registry.h

src/client/registry.cpp

src/client/xdgforeign.h

src/client/xdgforeign.cpp

src/client/xdgforeign_p.h

src/client/xdgforeign_v1.h

src/client/xdgforeign_v1.cpp

src/client/xdgforeign_v2.h

src/client/xdgforeign_v2.cpp

src/server/CMakeLists.txt

src/server/display.h

src/server/display.cpp

src/server/xdgforeign_interface.h

src/server/xdgforeign_interface.cpp

src/server/xdgforeign_v1_interface.cpp

src/server/xdgforeign_v1_interface_p.h

src/server/xdgforeign_v2_interface.cpp

src/server/xdgforeign_v2_interface_p.h

tests/CMakeLists.txt

tests/xdgforeigntest.cpp

Wayland foreign protocol
ClosedPublic
Actions

Revision Contents
Changeset List