Differential D28082

Take file descriptor only instead of whole KWayland Display
ClosedPublic
Actions

Authored by romangg on Mar 16 2020, 7:51 PM.

Details

Reviewers

davidedmundson

Group Reviewers

Plasma

Commits

R133:decf7c9d4f43: Take file descriptor only instead of whole KWayland Display

Summary

The current KScreenLocker expects the Compositor to hand over the
KWayland::Server::Display pointer.

This has several disadvantages:

In KScreenLocker functionality is duplicated which the compositor probably has setup already (creating client connections manually).
The Display object has a way larger scope than what KScreenLocker actually needs.
Ownership of the Display is unclear in regards to memory but also what the compositor is still allowed to do in comparision to KScreenLocker with the Display.
KScreenLocker can only be integrated in compositors using KWayland for managing their wl_display protocol object.

Instead it is now enough to hand over a single file descriptor KScreenLocker
can use as its client endpoint to talk to the compositor.

Test Plan

Manually in Wayland session, autotest.

Diff Detail

Branch

fd-only

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 23786
Build 23804: arc lint + arc unit

romangg requested review of this revision.Mar 16 2020, 7:51 PM

romangg created this revision.

Harbormaster completed remote builds in B23780: Diff 77778.Mar 16 2020, 7:51 PM

romangg added a dependency: D28081: Populate Greeter process exit stderr.Mar 16 2020, 7:52 PM

Rebase

Harbormaster completed remote builds in B23786: Diff 77785.Mar 16 2020, 9:53 PM

romangg added a dependent revision: D28085: Adapt to new KScreenLocker API.Mar 16 2020, 10:03 PM

+1 It's cool how the code is much simpler like this, good stuff.

ksldapp.cpp
650	Isn't this redundant with KSldApp::userActivity()?

romangg added inline comments.Mar 17 2020, 12:41 AM

ksldapp.cpp
650	Could be. I remember I thought about removing it but I refrained because it comes from m_lockWindow and I wasn't sure if there are not other triggers of user activity besides the seats timestamps. But might very well be redundant.

romangg added inline comments.Mar 17 2020, 3:23 AM

ksldapp.cpp
650	Ah wait, now I remember. In the X11 case we get the user activity from the X11Locker class (there is an event filter installed). That's also why I kept it here in the `if (m_isX11)` path.

romangg added inline comments.Mar 17 2020, 3:26 AM

ksldapp.cpp
650	What could be an idea therefore is to remove the connect and instead from the event filter in X11Locker directly call the userActivity function of KSldApp like in the Wayland case the compositor does. Would reduce the complexity definitely a bit more.

davidedmundson accepted this revision.Mar 18 2020, 9:39 PM

This revision is now accepted and ready to land.Mar 18 2020, 9:39 PM

Closed by commit R133:decf7c9d4f43: Take file descriptor only instead of whole KWayland Display (authored by romangg). · Explain WhyMar 28 2020, 9:38 PM

This revision was automatically updated to reflect the committed changes.

Restricted Application added a project: Plasma. · View Herald TranscriptMar 28 2020, 9:38 PM

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

Revision Contents
Changeset List

			Path	Packages
M			autotests/noscreens.cpp (55 lines)
M			ksldapp.h (20 lines)
M			ksldapp.cpp (79 lines)
M			waylandlocker.h (2 lines)
M			waylandlocker.cpp (9 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	77778	39bc5bd		Mar 16 2020, 7:50 PM	★	★
Diff 2	77785	f93594d	Rebase	Mar 16 2020, 9:53 PM	★	★
Diff 3	78761	c893e33	R133:decf7c9d4f4313ae20ae927d35ccf96ba29de7ae	Mar 28 2020, 9:41 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
af22f644b9e9	b940fe389670	f93594d2d345	Roman Gilg	Take file descriptor only instead of whole KWayland Display (Show More…)	Mar 15 2020, 9:45 PM

Status	Author	Revision
Closed	romangg	D28085 Adapt to new KScreenLocker API
Closed	romangg	D28082 Take file descriptor only instead of whole KWayland Display
Closed	romangg	D28081 Populate Greeter process exit stderr