Merge switch user dialog into lockscreen
ClosedPublic

Authored by davidedmundson on Aug 31 2018, 1:41 PM.

Details

Summary

This reduces a bunch of code, both hidden in the backend as well as the
mostly duplicated front end UI, making it more consistent for users too.

There is a behavioural change that switching user then cancelling will
require your own password. I would argue this is a good thing.

KSMServer still has the same DBus slot for compatibility which then
proxies over to the screensaver. This could be calling itself, it might
be calling kwin when we're on wayland.

Depends on D15186

Test Plan

Pressed switch user from the UI
Got a swich user dialog

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2388
Build 2406: arc lint + arc unit
davidedmundson created this revision.Aug 31 2018, 1:41 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 31 2018, 1:41 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Aug 31 2018, 1:41 PM
mart accepted this revision.Aug 31 2018, 1:57 PM
mart added a subscriber: mart.

lovely code deletions :)
and yeah, requiring the passowd again to cancel user switching kinda makes sense, it shouldn't be an issue

ksmserver/server.cpp
1095

should we need some confirmation from the screenlocker that the operation actually went well?

This revision is now accepted and ready to land.Aug 31 2018, 1:57 PM

Doesn't build for me:

make[2]: *** No rule to make target '/usr/share/dbus-1/interfaces/org.kde.screensaver.xml', needed by 'ksmserver/kscreenlocker_interface.cpp'.  Stop.
CMakeFiles/Makefile2:3454: recipe for target 'ksmserver/CMakeFiles/kdeinit_ksmserver.dir/all' failed
make[1]: *** [ksmserver/CMakeFiles/kdeinit_ksmserver.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 37%] Built target krunner
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

+1 on the idea though; the switch user screen has always been a bit unnerving for various reasons. I think this will be much nicer.

That said, I can foresee one potential issue: the interactive parts of the lockscreen UI aren't shown by default; you need to wiggle the mouse or bang on the keyboard to get anything interactive to show up. When we've entered the lockscreen in user switcher mode, I think the UI should be visible by default, since immediate user interaction is expected.

(if it already it, please ignore this part of the comment; I couldn't test it out because of the aforementioned compile error)

There's another patch today too. (Can't find it on my phone now)

But in any case it we never blur the switch user screen

There's another patch today too. (Can't find it on my phone now)

But in any case it we never blur the switch user screen

Perfect. No UI objections then!

davidedmundson added inline comments.Sep 1 2018, 7:29 AM
ksmserver/server.cpp
1095

good question.

The method is void, but we could still throw an error.

I checked all our code that calls this and they don't check the return.
If have to to change the calling code, I'll be changing it to call the method on the screensaver directly anyway.
Proxying here is just for compatibility.

This revision was automatically updated to reflect the committed changes.
bruns added a subscriber: bruns.Sep 3 2018, 1:01 AM
bruns added inline comments.
ksmserver/CMakeLists.txt
47

This breaks in two ways:

  • There is no specified dependency on KScreenlocker 5.13.80
  • One cannot build easily when dependencies have a different prefix as used for building plasma

The latter needs something like `set(KSCREENLOCKER_DBUS_INTERFACES_DIR "${PACKAGE_PREFIX_DIR}/share/dbus-1/interfaces")
` in KScreenLockerConfig.cmake
This is done by several other components which install DBUS interfaces (Solid, KWallet, Baloo, KNotifications).

bruns added a comment.Sep 3 2018, 1:19 AM

See D15228 for the KSCREENLOCKER_DBUS_INTERFACES_DIR part ...