D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[Pino Toscano]

pino added a comment.


  In D19389#443139 , @davidedmundson 
wrote:
  
  > Sessionbackend.h is not installed.
  
  
  sessionmanagement.h (not sessionbackend.h) is indeed installed, according to 
your patch. Considering that D20237: Port to new KWorkspace API 
 is in another repository, then making it a 
public header is the only sane choice (please do not copy it, it would be even 
worse).

REPOSITORY
  R120 Plasma Workspace

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

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[David Edmundson]

davidedmundson added a comment.


  Sessionbackend.h is not installed.
  
  I should rename it _p.h maybe

REPOSITORY
  R120 Plasma Workspace

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

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[Pino Toscano]

pino added a comment.


  - loginddbustypes.h requires a license header
  - SessionBackend needs a d-pointer to minimize BIC changes
  - I'd move all the SessionBackend subclasses in a private header; they are 
not exported anyway, so unusable for users of this public library

REPOSITORY
  R120 Plasma Workspace

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

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[David Edmundson]

davidedmundson updated this revision to Diff 55363.
davidedmundson added a comment.


  Wrap saveSession

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19389?vs=55319=55363

BRANCH
  origin-master (branched from master)

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

AFFECTED FILES
  libkworkspace/CMakeLists.txt
  libkworkspace/kworkspace.cpp
  libkworkspace/kworkspace_p.h
  libkworkspace/login1_manager_interface.cpp
  libkworkspace/loginddbustypes.h
  libkworkspace/org.freedesktop.ConsoleKit.Manager.xml
  libkworkspace/org.freedesktop.UPower.xml
  libkworkspace/org.freedesktop.login1.Manager.xml
  libkworkspace/org.freedesktop.login1.Seat.xml
  libkworkspace/org.freedesktop.login1.Session.xml
  libkworkspace/org.freedesktop.login1.User.xml
  libkworkspace/sessionmanagement.cpp
  libkworkspace/sessionmanagement.h
  libkworkspace/sessionmanagementbackend.cpp
  libkworkspace/sessionmanagementbackend.h
  libkworkspace/tests/CMakeLists.txt
  libkworkspace/tests/sessiontest.cpp

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[David Edmundson]

davidedmundson updated this revision to Diff 55319.
davidedmundson marked 3 inline comments as done.
davidedmundson added a comment.


  updates

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19389?vs=53111=55319

BRANCH
  master

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

AFFECTED FILES
  libkworkspace/CMakeLists.txt
  libkworkspace/kworkspace.cpp
  libkworkspace/kworkspace_p.h
  libkworkspace/login1_manager_interface.cpp
  libkworkspace/loginddbustypes.h
  libkworkspace/org.freedesktop.ConsoleKit.Manager.xml
  libkworkspace/org.freedesktop.UPower.xml
  libkworkspace/org.freedesktop.login1.Manager.xml
  libkworkspace/org.freedesktop.login1.Seat.xml
  libkworkspace/org.freedesktop.login1.Session.xml
  libkworkspace/org.freedesktop.login1.User.xml
  libkworkspace/sessionmanagement.cpp
  libkworkspace/sessionmanagement.h
  libkworkspace/sessionmanagementbackend.cpp
  libkworkspace/sessionmanagementbackend.h
  libkworkspace/tests/CMakeLists.txt
  libkworkspace/tests/sessiontest.cpp

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[Kai Uwe Broulik]

broulik added a comment.


  +1
  Nice and clean, no real complaints.

INLINE COMMENTS

> kworkspace.cpp:70
>  
> +void requestShutDown(KWorkSpace::ShutdownConfirm, KWorkSpace::ShutdownType, 
> KWorkSpace::ShutdownMode)
> +{

:)

> sessionmanagement.cpp:32
> +
> +// add a constructor with the servica names and paths pre-populated
> +class LogoutPromptIface : public OrgKdeLogoutPromptInterface {

*service

> sessionmanagement.cpp:95
> +{
> +return KAuthorized::authorizeAction(QStringLiteral("start_new_session"));
> +}

The old one also checked KDM or whatever for "free ttys", is that still a thing?

> sessionmanagementbackend.cpp:46
> +s_backend = new LogindSessionBackend();
> +} else if (ConsoleKitSessionBackend::exists() ){
> +s_backend = new ConsoleKitSessionBackend();

Coding style

> sessionmanagementbackend.cpp:51
> +}
> +Q_ASSERT(s_backend);
> +

Can this even be hit?

> sessionmanagementbackend.cpp:84
> +} else {
> +// DECISION FIXME, do we want to show the option in the menu if 
> the response is "challenge"?
> +*argToUpdate = reply.value() == QLatin1String("yes")  ? true : 
> false;

I would say yes. If one wants to hide from UI they can use KIOSK? 
Docs perhaps need updating that Polkit is obviously prefered since kiosk is 
mostly UI and not real security

REPOSITORY
  R120 Plasma Workspace

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

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[David Edmundson]

davidedmundson updated this revision to Diff 53111.
davidedmundson added a comment.


  add hybrid suspend

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19389?vs=52736=53111

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

AFFECTED FILES
  libkworkspace/CMakeLists.txt
  libkworkspace/kworkspace.cpp
  libkworkspace/kworkspace_p.h
  libkworkspace/login1_manager_interface.cpp
  libkworkspace/loginddbustypes.h
  libkworkspace/org.freedesktop.ConsoleKit.Manager.xml
  libkworkspace/org.freedesktop.UPower.xml
  libkworkspace/org.freedesktop.login1.Manager.xml
  libkworkspace/org.freedesktop.login1.Seat.xml
  libkworkspace/org.freedesktop.login1.Session.xml
  libkworkspace/org.freedesktop.login1.User.xml
  libkworkspace/sessionmanagement.cpp
  libkworkspace/sessionmanagement.h
  libkworkspace/sessionmanagementbackend.cpp
  libkworkspace/sessionmanagementbackend.h
  libkworkspace/tests/CMakeLists.txt
  libkworkspace/tests/sessiontest.cpp

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[David Edmundson]

davidedmundson updated this revision to Diff 52736.
davidedmundson added a comment.


  update one line

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19389?vs=52733=52736

BRANCH
  davidedmundson/kworkspace2

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

AFFECTED FILES
  libkworkspace/CMakeLists.txt
  libkworkspace/kworkspace.cpp
  libkworkspace/kworkspace_p.h
  libkworkspace/login1_manager_interface.cpp
  libkworkspace/loginddbustypes.h
  libkworkspace/org.freedesktop.ConsoleKit.Manager.xml
  libkworkspace/org.freedesktop.UPower.xml
  libkworkspace/org.freedesktop.login1.Manager.xml
  libkworkspace/org.freedesktop.login1.Seat.xml
  libkworkspace/org.freedesktop.login1.Session.xml
  libkworkspace/org.freedesktop.login1.User.xml
  libkworkspace/sessionmanagement.cpp
  libkworkspace/sessionmanagement.h
  libkworkspace/sessionmanagementbackend.cpp
  libkworkspace/sessionmanagementbackend.h
  libkworkspace/tests/CMakeLists.txt
  libkworkspace/tests/sessiontest.cpp

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

D19389: [WIP] Rewrite kworkspace logout, shutdown and suspend API

[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
  The old code path is old and even sometimes uses the ICE protocol to
  start a logout from ksmserver. It's quite archaic and unreadable,
  KDisplayManager is even worse.
  
  The new code consists of two parts.
  
  A QML friendly front end QObject that we can register in a plugin that
  can trigger actions such as prompting a logout or suspending the system
  and a singleton that talks to the relevant backend.
  
  Backends are:
  
  - Any logind interface
  - Consolekit2
  - Consolekit0.4 + UPower
  
  New API loading is also entirely asyncronous.
  
  There are some behavioural changes:
  
  - Creating a logout prompt is called directly instead of going via
  
  ksmserver. The prompt then calls into ksmserver when accepted
  
  - Suspend is called directly instead of going via powerdevil through
  
  some kdelibs4support solid code.
  
  All DBus calls use generated XML files, instead of big QDBusMessage
  commands. It's a bit overkill in terms of what we generate, but that's
  ready for moving SessionModel in here and killing KDisplayManager.
  
  Patch looks huge because of all the XML files, but is otherwise very
  simple.
  
  This is intended to be an API break, which is fine as kworkspace is only
  to be used by plasma. The relevant DataSource will remain compatiable.
  
  WIP: As I need to port all the calling code, and I want to port
  SessionsModel (probably in a different patch)

TEST PLAN
  Ran sessionstest on my logind machine
  I do need other people to test their setups. 
  Especially a BSD person with CK1

REPOSITORY
  R120 Plasma Workspace

BRANCH
  davidedmundson/kworkspace2

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

AFFECTED FILES
  libkworkspace/CMakeLists.txt
  libkworkspace/kworkspace.cpp
  libkworkspace/kworkspace_p.h
  libkworkspace/login1_manager_interface.cpp
  libkworkspace/loginddbustypes.h
  libkworkspace/org.freedesktop.ConsoleKit.Manager.xml
  libkworkspace/org.freedesktop.UPower.xml
  libkworkspace/org.freedesktop.login1.Manager.xml
  libkworkspace/org.freedesktop.login1.Seat.xml
  libkworkspace/org.freedesktop.login1.Session.xml
  libkworkspace/org.freedesktop.login1.User.xml
  libkworkspace/sessionmanagement.cpp
  libkworkspace/sessionmanagement.h
  libkworkspace/sessionmanagementbackend.cpp
  libkworkspace/sessionmanagementbackend.h
  libkworkspace/tests/CMakeLists.txt
  libkworkspace/tests/sessiontest.cpp

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