D15187: Merge switch user dialog into lockscreen

[Stefan Brüns]

bruns added a comment.


  See D15228  for the 
KSCREENLOCKER_DBUS_INTERFACES_DIR part ...

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: bruns, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> CMakeLists.txt:47
>  
> +qt5_add_dbus_interface( ksmserver_KDEINIT_SRCS 
> ${KDE_INSTALL_FULL_DBUSINTERFACEDIR}/org.kde.screensaver.xml 
> kscreenlocker_interface )
> +

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).

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: bruns, ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:084b9a408cda: Merge switch user dialog into lockscreen 
(authored by davidedmundson).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D15187?vs=40768=40806#toc

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15187?vs=40768=40806

REVISION DETAIL
  https://phabricator.kde.org/D15187

AFFECTED FILES
  ksmserver/CMakeLists.txt
  ksmserver/server.cpp
  ksmserver/switchuser-greeter/CMakeLists.txt
  ksmserver/switchuser-greeter/main.cpp
  lookandfeel/contents/userswitcher/UserSwitcher.qml

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> mart wrote in server.cpp:1095
> should we need some confirmation from the screenlocker that the operation 
> actually went well?

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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[David Edmundson]

davidedmundson added a comment.


  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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[Nathaniel Graham]

ngraham added a comment.


  In D15187#318464 , @davidedmundson 
wrote:
  
  > 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!

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[David Edmundson]

davidedmundson added a comment.


  See 15186

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[Nathaniel Graham]

ngraham added a comment.


  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)

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: ngraham, mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[Marco Martin]

mart accepted this revision.
mart added a comment.
This revision is now accepted and ready to land.


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

INLINE COMMENTS

> server.cpp:1095
> +OrgKdeScreensaverInterface 
> iface(QStringLiteral("org.freedesktop.ScreenSaver"), 
> QStringLiteral("/ScreenSaver"), QDBusConnection::sessionBus());
> +iface.SwitchUser();
>  }

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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

To: davidedmundson, #plasma, mart
Cc: mart, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D15187: Merge switch user dialog into lockscreen

[David Edmundson]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
davidedmundson requested review of this revision.

REVISION 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.

TEST PLAN
  Pressed switch user from the UI
  Got a swich user dialog

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D15187

AFFECTED FILES
  ksmserver/CMakeLists.txt
  ksmserver/server.cpp
  ksmserver/switchuser-greeter/CMakeLists.txt
  ksmserver/switchuser-greeter/main.cpp
  lookandfeel/contents/userswitcher/UserSwitcher.qml

To: davidedmundson, #plasma
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart