Take file descriptor only instead of whole KWayland Display
ClosedPublic

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

Details

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

Repository
R133 KScreenLocker
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg requested review of this revision.Mon, Mar 16, 7:51 PM
romangg created this revision.
apol added a subscriber: apol.Tue, Mar 17, 12:33 AM

+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.Tue, Mar 17, 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.Tue, Mar 17, 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.Tue, Mar 17, 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.Wed, Mar 18, 9:39 PM
This revision is now accepted and ready to land.Wed, Mar 18, 9:39 PM
This revision was automatically updated to reflect the committed changes.
Restricted Application added a project: Plasma. · View Herald TranscriptSat, Mar 28, 9:38 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript