Details

Reviewers

Group Reviewers

Commits

R127:905e699677ed: Implement wl_text_input and zwp_text_input_v2 interfaces

Summary

This change introduces support for text input. Text input allows to
compose text on the server (e.g. through a virtual keyboard) and sent
the composed text to the client.

There are multiple interfaces for text input. QtWayland 5.6 uses
wl_text_input, QtWayland 5.7 uses zwp_text_input_v2.

wl_text_input is from pre Wayland-Protocols times and considered as
UnstableV0 in this implementation. The other interface is UnstableV2.
Unfortunately the V2 variant is not yet part of Wayland-Protocols, but
used in Qt.

The implementation hides the different interfaces as good as possible.
The general idea is the same, the differences are rather minor.

This means changes to how interfaces are wrapped normally. On client
side in the Registry a manager is factored which represent either of
the two interfaces. Similar on the server side Display's factory method
takes an argument to decide which interface should be factored. This
way a user of the library can expose both interfaces and thus be
compatible with Qt 5.6 and Qt 5.7 onwards.

Diff Detail

Repository

R127 KWayland

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

graesslin updated this revision to Diff 3860.May 18 2016, 3:36 PM

graesslin retitled this revision from to Implement wl_text_input and zwp_text_input_v2 interfaces.

graesslin updated this object.

graesslin edited the test plan for this revision. (Show Details)

graesslin added a reviewer: Plasma.

Restricted Application added a project: Plasma. · View Herald TranscriptMay 18 2016, 3:36 PM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Sorry for the large review. The fact that it supports two interfaces made it rather complex.

When reviewing please check whether the exposed methods sound good, are well documented, etc. Please also verify whether the implementation actually matches what is described in the two xml protocols.

More documentation

Some method renaming:

on client: activate -> enable, deactivate -> disable
on server: made setters not sound like getters

graesslin added a dependent revision: D1638: Integrate QtVirtualKeyboard into KWin/Wayland.May 19 2016, 10:35 AM

broulik added a subscriber: broulik.May 19 2016, 1:30 PM

broulik added inline comments.

src/server/textinput_interface.h
159	MultiLine? Makes it consistent with Qt API

graesslin marked an inline comment as done.May 20 2016, 5:42 AM

Multiline -> MultiLine

The SeatInterface now exposes TextInputInterface in similar ways to the other
input devices. That is there is a setFocusedTextInputSurface and the focused
TextInputInterface is exposed through focusedTextInput().

This means that text input is no longer strictly bound to keyboard focus, it
can be set indipendently. Though setFocusedKeyboardSurface updates the
focusedTextInputSurface if the SeatInterface has keyboard support. This is
a requirement in the v2 xml description.

In addition TextInputInterface has a new method isEnabled() -> bool to check
whether it's currently enabled and emits a signal if this changes. This can
be used by the compositor to decide whether it should send text input through
the interface.

include QVector in one header to make it compile with Qt 5.6

a few niggles, but generally, look good to me.

src/client/outputmanagement.cpp
94 ↗	(On Diff #3990)	Seems unrelated (not a problem, unless I'm missing something)
src/client/plasmawindowmanagement.cpp
128 ↗	(On Diff #3990)	Seems unrelated (not a problem, unless I'm missing something)
357 ↗	(On Diff #3990)	Seems unrelated (not a problem, unless I'm missing something)
src/client/plasmawindowmodel.cpp
60 ↗	(On Diff #3990)	the changes in this file are unrelated, but do make sense (I don't care much if you push them together)
src/client/protocols/plasma-window-management.xml
20 ↗	(On Diff #3990)	changes here are also unrelated?
src/client/protocols/text-input-unstable-v2.xml
154	"a URL"
src/client/registry.h
451	A small note about the protocol (i.e. which version of the interface should be used) probably wouldn't hurt (as the two creators may end up being confusing). This is a bit high-level though, for APIDOCs, but still raises this question.
src/client/textinput.h
50	move this above the previous paragraph, more logical order (general purpose, then implementation caveats)
82	Does this actually show the panel then? (Could be more clear what's expected here, what's the relationship between enable/disable and show/hide)
158	I think LowerCase, UpperCase and TitleCase feel more in line with the other fields here?
src/server/textinput_interface.h
135	LowerCase, UpperCase, etc. as well?
217	DateTime?

This revision is now accepted and ready to land.May 25 2016, 12:20 PM

Upload new version against master

graesslin marked an inline comment as done.May 25 2016, 12:33 PM

graesslin added inline comments.

src/client/protocols/text-input-unstable-v2.xml
154	this is an XML file I imported from 3rd party (QtWayland). Not going to touch it.
src/client/registry.h
451	Isn't the explanation in createTextInputManager sufficient? After all it says one should prefer createTextInputManager
src/client/textinput.h
82	honest answer: I don't know.

graesslin marked 3 inline comments as done.May 25 2016, 12:37 PM

Addresses sebas review comments:

fixed documentation in Registry
Reordered documentation in TextInput
Changed enum values to CamelCase

ship it!

Closed by commit R127:905e699677ed: Implement wl_text_input and zwp_text_input_v2 interfaces (authored by graesslin). · Explain WhyMay 25 2016, 1:11 PM

This revision was automatically updated to reflect the committed changes.

		Path
M		autotests/client/CMakeLists.txt (11 lines)
A	M	autotests/client/test_text_input.cpp (917 lines)
M		autotests/client/test_wayland_registry.cpp (25 lines)
M		src/client/CMakeLists.txt (12 lines)
A	M	src/client/protocols/text-input-unstable-v2.xml (478 lines)
A	M	src/client/protocols/text-input.xml (346 lines)
M		src/client/registry.h (72 lines)
M		src/client/registry.cpp (44 lines)
A	M	src/client/textinput.h (536 lines)
A	M	src/client/textinput.cpp (228 lines)
A	M	src/client/textinput_p.h (245 lines)
A	M	src/client/textinput_v0.cpp (500 lines)
A	M	src/client/textinput_v2.cpp (516 lines)
M		src/server/CMakeLists.txt (14 lines)
M		src/server/display.h (8 lines)
M		src/server/display.cpp (18 lines)
M		src/server/seat_interface.h (63 lines)
M		src/server/seat_interface.cpp (90 lines)
M		src/server/seat_interface_p.h (15 lines)
A	M	src/server/textinput_interface.h (439 lines)
A	M	src/server/textinput_interface.cpp (411 lines)
A	M	src/server/textinput_interface_p.h (166 lines)
A	M	src/server/textinput_interface_v0.cpp (313 lines)
A	M	src/server/textinput_interface_v2.cpp (324 lines)
M		src/tools/mapping.txt (4 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3860	5027955		May 18 2016, 3:36 PM	★	★
Diff 2	3872	5027955	More documentation	May 19 2016, 6:06 AM	★	★
Diff 3	3873	5027955	Some method renaming:	May 19 2016, 6:19 AM	★	★
Diff 4	3899	5027955	Multiline -> MultiLine	May 20 2016, 5:42 AM	★	★
Diff 5	3903	5027955	The SeatInterface now exposes TextInputInterface in similar ways to the other	May 20 2016, 8:45 AM	★	★
Diff 6	3990	9a689ad	include QVector in one header to make it compile with Qt 5.6	May 25 2016, 11:19 AM	★	★
Diff 7	3995	a370b07	Upload new version against master	May 25 2016, 12:27 PM	★	★
Diff 8	3997	a370b07	Addresses sebas review comments:	May 25 2016, 12:38 PM	★	★
Diff 9	3998	0534c5d	rKWAYLAND905e699677edc6d68694a13df7bb4bad692326a8	May 25 2016, 1:10 PM	★	★

	Status	Author	Revision
	Closed	graesslin	D1638 Integrate QtVirtualKeyboard into KWin/Wayland
	Closed	graesslin	D1631 Implement wl_text_input and zwp_text_input_v2 interfaces

Implement wl_text_input and zwp_text_input_v2 interfaces
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 3998

autotests/client/CMakeLists.txt

autotests/client/test_text_input.cpp

autotests/client/test_wayland_registry.cpp

src/client/CMakeLists.txt

src/client/protocols/text-input-unstable-v2.xml

src/client/protocols/text-input.xml

src/client/registry.h

src/client/registry.cpp

src/client/textinput.h

src/client/textinput.cpp

src/client/textinput_p.h

src/client/textinput_v0.cpp

src/client/textinput_v2.cpp

src/server/CMakeLists.txt

src/server/display.h

src/server/display.cpp

src/server/seat_interface.h

src/server/seat_interface.cpp

src/server/seat_interface_p.h

src/server/textinput_interface.h

src/server/textinput_interface.cpp

src/server/textinput_interface_p.h

src/server/textinput_interface_v0.cpp

src/server/textinput_interface_v2.cpp

src/tools/mapping.txt

Implement wl_text_input and zwp_text_input_v2 interfacesClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 3998

autotests/client/CMakeLists.txt

autotests/client/test_text_input.cpp

autotests/client/test_wayland_registry.cpp

src/client/CMakeLists.txt

src/client/protocols/text-input-unstable-v2.xml

src/client/protocols/text-input.xml

src/client/registry.h

src/client/registry.cpp

src/client/textinput.h

src/client/textinput.cpp

src/client/textinput_p.h

src/client/textinput_v0.cpp

src/client/textinput_v2.cpp

src/server/CMakeLists.txt

src/server/display.h

src/server/display.cpp

src/server/seat_interface.h

src/server/seat_interface.cpp

src/server/seat_interface_p.h

src/server/textinput_interface.h

src/server/textinput_interface.cpp

src/server/textinput_interface_p.h

src/server/textinput_interface_v0.cpp

src/server/textinput_interface_v2.cpp

src/tools/mapping.txt

Implement wl_text_input and zwp_text_input_v2 interfaces
ClosedPublic
Actions

Revision Contents
Changeset List