Differential D1594

Improve the deconstruction of PlasmaWindows
ClosedPublic
Actions

Authored by graesslin on May 12 2016, 5:48 AM.

Details

Reviewers

hein

Group Reviewers

Plasma

Commits

R127:866aea1a93db: Improve the deconstruction of PlasmaWindows

Summary

The protocol is extended by a dedicated destructor request. When a
PlasmaWindow is umapped we no longer destroy the resource directly,
but only send the unmap. The client is then supposed to clean up
(which it already did in that case) and will invoke the destructor.

The PlasmaWindowInterface object will be automatically deleted after
the unmap once all resources bound for it are destroyed.

The tests are extended by two new test cases which triggered protocol
errors on the client side prior to this change.

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 3773.May 12 2016, 5:48 AM

graesslin retitled this revision from to Improve the deconstruction of PlasmaWindows.

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 12 2016, 5:48 AM

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

graesslin added a dependent revision: D1595: Unmap PlasmaWindowInterface instead of deleting directly.May 12 2016, 6:00 AM

hein added a subscriber: hein.May 12 2016, 7:29 AM

hein added inline comments.

src/server/plasmawindowmanagement_interface.cpp
252	Can window ever be in the list more than once? I tend to dislike removeAll() because it can hide multi-insert code bugs, so if we can use removeOne and have an autotest somehow veryifying the integrity of the list it might be nicer. Minor nitpick though.

graesslin added inline comments.May 12 2016, 8:42 AM

src/server/plasmawindowmanagement_interface.cpp
252	No windows cannot be in it multiple times. It's only insert from one place where a new object is created and added. So, one could use removeOne and then a Q_ASSERT for contains.

use removeOne + Q_ASSERT, also reorder cleanup to have client destroy before server, otherwise crash possible

hein accepted this revision.May 12 2016, 8:56 AM

hein added a reviewer: hein.

This revision is now accepted and ready to land.May 12 2016, 8:56 AM

Closed by commit R127:866aea1a93db: Improve the deconstruction of PlasmaWindows (authored by graesslin). · Explain WhyMay 12 2016, 9:07 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			autotests/client/test_wayland_windowmanagement.cpp (50 lines)
M			src/client/protocols/plasma-window-management.xml (10 lines)
M			src/client/registry.cpp (2 lines)
M			src/server/plasmawindowmanagement_interface.h (24 lines)
M			src/server/plasmawindowmanagement_interface.cpp (88 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	3773	1d531e9		May 12 2016, 5:48 AM	★	★
Diff 2	3778	1d531e9	use removeOne + Q_ASSERT, also reorder cleanup to have client destroy before server, otherwise crash possible	May 12 2016, 8:55 AM	★	★
Diff 3	3779	1d531e9	rKWAYLAND866aea1a93db594c619735930c0aeca011dcd3ea	May 12 2016, 8:55 AM	★	★