Support passing generic QIcon through PlasmaWindow interface
ClosedPublic
Actions

Authored by graesslin on Oct 13 2016, 11:57 AM.

Details

Reviewers

roberts
romangg
hein

Group Reviewers

Plasma on Wayland

Commits

R127:bd2b966523e7: Support passing generic QIcon through PlasmaWindow interface

Summary

Especially for Xwayland windows the compositor might not have a themed
icon name. Resulting in a task manager not having dedicated icons for
Xwayland windows.

This change deprecates the way how a compositor is supposed to set the
window icon. Instead of passing the themed icon name, it is now supposed to
pass the QIcon. In case it's a themed icon the existing way to pass to
the client is used.

Otherwise a new event is used to inform the client that there is an icon

no data is transmitted at this point. The client can then create a

file descriptor and pass it to the compositor. The compositor serializes
the icon into the file descriptor and the client can read from it. This
all happens transparently on client side there is no api change at all.

The writing and reading of the icon is done in a thread. Due to that
Qt5::Concurrent is now a required dependency instead of an optional
dependency.

Diff Detail

Repository

R127 KWayland

Branch

plasma-window-icon

Lint

No Linters Available

Unit

No Unit Test Coverage

graesslin updated this revision to Diff 7376.Oct 13 2016, 11:57 AM

graesslin retitled this revision from to Support passing generic QIcon through PlasmaWindow interface.

graesslin updated this object.

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

graesslin added reviewers: Plasma on Wayland, hein.

Restricted Application added a project: Plasma on Wayland. · View Herald TranscriptOct 13 2016, 11:57 AM

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

graesslin added a dependent revision: D3050: Use new PlasmaWindowInterface::setIcon.Oct 13 2016, 11:58 AM

roberts added a subscriber: roberts.Oct 13 2016, 6:32 PM

roberts added inline comments.

src/client/plasmawindowmanagement.cpp
489	EAGAIN comes from reads of O_NONBLOCK fd's, but this seems to be a blocking pipe (no calls to fcntl() or pipe2()). As such the read call could block the thread for significant or unbounded time - depending on whether QT_READ swallows EINTR - if the other side of the pipe failed to close the fd.
516	pipeFd could leak here.
src/server/plasmawindowmanagement_interface.cpp
406	Is there anything protecting this from being unsynchronised parallel access to m_icon (and p)? Simplest solution might be passing the icon by value into the lambda.

graesslin added inline comments.Oct 14 2016, 8:36 AM

src/server/plasmawindowmanagement_interface.cpp
406	I assumed that reading from the icon is thread save. But you are right: better save than sorry. Especially accessing the p pointer could be dangerous as the PlasmaWindowInterface instance might get deleted.

graesslin marked 3 inline comments as done.Oct 14 2016, 8:42 AM

Pass icon as argument to the runnable
use pipe2
close pipefd in error handling branch

Looks good to me now.

This revision is now accepted and ready to land.Oct 14 2016, 6:12 PM

apol added a subscriber: apol.Oct 14 2016, 8:56 PM

apol added inline comments.

src/server/plasmawindowmanagement_interface.cpp
386	maybe worth adding a `if (m_icon == icon) return;`

graesslin added inline comments.Oct 15 2016, 12:09 PM

src/server/plasmawindowmanagement_interface.cpp
386	in theory yes, in practice: operator== doesn't exist for QIcon so all you get is a compile error :-(

Told you, I had problems compiling at first. Works now for unknown reason and icons are displayed together with D3050.

src/server/plasmawindowmanagement_interface.cpp
386	What about comparing QIcon::cacheKey() values?

Closed by commit R127:bd2b966523e7: Support passing generic QIcon through PlasmaWindow interface (authored by graesslin). · Explain WhyOct 17 2016, 5:50 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			CMakeLists.txt (2 lines)
M			autotests/client/test_wayland_windowmanagement.cpp (38 lines)
M			src/client/CMakeLists.txt (1 line)
M			src/client/plasmawindowmanagement.cpp (64 lines)
M			src/client/protocols/plasma-window-management.xml (21 lines)
M			src/client/registry.cpp (2 lines)
M			src/server/CMakeLists.txt (1 line)
M			src/server/plasmawindowmanagement_interface.h (21 lines)
M			src/server/plasmawindowmanagement_interface.cpp (54 lines)
M			src/tools/CMakeLists.txt (15 lines)
M			tests/CMakeLists.txt (11 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	7376	71ef897		Oct 13 2016, 11:57 AM	★	★
Diff 2	7384	71ef897	* Pass icon as argument to the runnable	Oct 14 2016, 8:42 AM	★	★
Diff 3	7433	fe95610	rKWAYLANDbd2b966523e74cea812c8affae4eb62e39b29952	Oct 17 2016, 5:35 AM	★	★