From ec610fd7edb7aecdd170037fe35997951533b8f5 Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Thu, 31 Oct 2019 16:16:53 +0000 Subject: [PATCH] Port one of session management connections state to a custom API Summary: Currently kwin opens a second ICE connection to ksmserver in order to tell the state of kwin's whether we're logging out and saving clients or not. This requires that kwin launches after ksmserver to have the connection which is a dependency I want to break. Practically this code is already ksmserver specific as it relies on some custom code that sends the first saveState request to kwin first. Instead we can replace it with a bespoke IPC over DBus and siplify the code both end. This will allow several other future enhancements that we want with regards to handling the session state, as well as make an effort platform agnostic session management, as well as cleaning up some complex code. Ksmserver calls into kwin, rather than having kwin watch ksmserver state to allow us make sure it's race free. Reviewers: #kwin, zzag Reviewed By: #kwin, zzag Subscribers: romangg, zzag, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D24862 --- CMakeLists.txt | 1 + activation.cpp | 4 +- activities.cpp | 6 +- libkwineffects/kwinglobals.h | 10 ++ main_x11.cpp | 3 - manage.cpp | 9 +- org.kde.KWin.Session.xml | 15 +++ sm.cpp | 210 +++++++++-------------------------- sm.h | 44 ++++---- workspace.cpp | 3 +- workspace.h | 18 +-- 11 files changed, 114 insertions(+), 209 deletions(-) create mode 100644 org.kde.KWin.Session.xml diff --git a/CMakeLists.txt b/CMakeLists.txt index 84286a0326..b1dccce4ea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -549,6 +549,7 @@ qt5_add_dbus_adaptor(kwin_KDEINIT_SRCS org.kde.kwin.ColorCorrect.xml colorcorrec qt5_add_dbus_adaptor(kwin_KDEINIT_SRCS ${kwin_effects_dbus_xml} effects.h KWin::EffectsHandlerImpl) qt5_add_dbus_adaptor(kwin_KDEINIT_SRCS org.kde.kwin.OrientationSensor.xml orientation_sensor.h KWin::OrientationSensor) qt5_add_dbus_adaptor(kwin_KDEINIT_SRCS org.kde.KWin.VirtualDesktopManager.xml dbusinterface.h KWin::VirtualDesktopManagerDBusInterface) +qt5_add_dbus_adaptor(kwin_KDEINIT_SRCS org.kde.KWin.Session.xml sm.h KWin::SessionManager) qt5_add_dbus_interface(kwin_KDEINIT_SRCS ${KSCREENLOCKER_DBUS_INTERFACES_DIR}/kf5_org.freedesktop.ScreenSaver.xml screenlocker_interface) qt5_add_dbus_interface(kwin_KDEINIT_SRCS ${KSCREENLOCKER_DBUS_INTERFACES_DIR}/org.kde.screensaver.xml kscreenlocker_interface) diff --git a/activation.cpp b/activation.cpp index 09a2cfce64..f98b25e96e 100644 --- a/activation.cpp +++ b/activation.cpp @@ -558,7 +558,7 @@ bool Workspace::allowClientActivation(const KWin::AbstractClient *c, xcb_timesta if (time == -1U) time = c->userTime(); int level = c->rules()->checkFSP(options->focusStealingPreventionLevel()); - if (session_saving && level <= FSP::Medium) { // <= normal + if (sessionManager()->state() == SessionState::Saving && level <= FSP::Medium) { // <= normal return true; } AbstractClient* ac = mostRecentlyActivatedClient(); @@ -635,7 +635,7 @@ bool Workspace::allowClientActivation(const KWin::AbstractClient *c, xcb_timesta bool Workspace::allowFullClientRaising(const KWin::AbstractClient *c, xcb_timestamp_t time) { int level = c->rules()->checkFSP(options->focusStealingPreventionLevel()); - if (session_saving && level <= 2) { // <= normal + if (sessionManager()->state() == SessionState::Saving && level <= 2) { // <= normal return true; } AbstractClient* ac = mostRecentlyActivatedClient(); diff --git a/activities.cpp b/activities.cpp index 2f264d4047..67418915b7 100644 --- a/activities.cpp +++ b/activities.cpp @@ -121,7 +121,7 @@ void Activities::toggleClientOnActivity(X11Client *c, const QString &activity, b bool Activities::start(const QString &id) { Workspace *ws = Workspace::self(); - if (ws->sessionSaving()) { + if (ws->sessionManager()->state() == SessionState::Saving) { return false; //ksmserver doesn't queue requests (yet) } @@ -143,7 +143,7 @@ bool Activities::start(const QString &id) bool Activities::stop(const QString &id) { - if (Workspace::self()->sessionSaving()) { + if (Workspace::self()->sessionManager()->state() == SessionState::Saving) { return false; //ksmserver doesn't queue requests (yet) //FIXME what about session *loading*? } @@ -157,7 +157,7 @@ bool Activities::stop(const QString &id) void Activities::reallyStop(const QString &id) { Workspace *ws = Workspace::self(); - if (ws->sessionSaving()) + if (ws->sessionManager()->state() == SessionState::Saving) return; //ksmserver doesn't queue requests (yet) qCDebug(KWIN_CORE) << id; diff --git a/libkwineffects/kwinglobals.h b/libkwineffects/kwinglobals.h index 649575444d..18d352374c 100644 --- a/libkwineffects/kwinglobals.h +++ b/libkwineffects/kwinglobals.h @@ -138,6 +138,16 @@ enum class SwipeDirection { Right }; +/** + * Represents the state of the session running outside kwin + * Under Plasma this is managed by ksmserver + */ +enum class SessionState { + Normal, + Saving, + Quitting +}; + inline KWIN_EXPORT xcb_connection_t *connection() { diff --git a/main_x11.cpp b/main_x11.cpp index 67dc066697..ff6184fe31 100644 --- a/main_x11.cpp +++ b/main_x11.cpp @@ -480,9 +480,6 @@ KWIN_EXPORT int kdemain(int argc, char * argv[]) a.start(); - KWin::SessionSaveDoneHelper helper; - Q_UNUSED(helper); // The sessionsavedonehelper opens a side channel to the smserver, - // listens for events and talks to it, so it needs to be created. return a.exec(); } diff --git a/manage.cpp b/manage.cpp index b711651f06..9741f4390b 100644 --- a/manage.cpp +++ b/manage.cpp @@ -432,7 +432,8 @@ bool X11Client::manage(xcb_window_t w, bool isMapped) init_minimize = false; // SELI TODO: Even e.g. for NET::Utility? } // If a dialog is shown for minimized window, minimize it too - if (!init_minimize && isTransient() && mainClients().count() > 0 && !workspace()->sessionSaving()) { + if (!init_minimize && isTransient() && mainClients().count() > 0 && + workspace()->sessionManager()->state() != SessionState::Saving) { bool visible_parent = false; // Use allMainClients(), to include also main clients of group transients // that have been optimized out in X11Client::checkGroupTransients() @@ -532,13 +533,15 @@ bool X11Client::manage(xcb_window_t w, bool isMapped) else allow = workspace()->allowClientActivation(this, userTime(), false); + const bool isSessionSaving = workspace()->sessionManager()->state() == SessionState::Saving; + // If session saving, force showing new windows (i.e. "save file?" dialogs etc.) // also force if activation is allowed - if( !isOnCurrentDesktop() && !isMapped && !session && ( allow || workspace()->sessionSaving() )) + if( !isOnCurrentDesktop() && !isMapped && !session && ( allow || isSessionSaving )) VirtualDesktopManager::self()->setCurrent( desktop()); // If the window is on an inactive activity during session saving, temporarily force it to show. - if( !isMapped && !session && workspace()->sessionSaving() && !isOnCurrentActivity()) { + if( !isMapped && !session && isSessionSaving && !isOnCurrentActivity()) { setSessionActivityOverride( true ); foreach( AbstractClient* c, mainClients()) { if (X11Client *mc = dynamic_cast(c)) { diff --git a/org.kde.KWin.Session.xml b/org.kde.KWin.Session.xml new file mode 100644 index 0000000000..279c24d064 --- /dev/null +++ b/org.kde.KWin.Session.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + diff --git a/sm.cpp b/sm.cpp index 285973524e..e1aa8bca27 100644 --- a/sm.cpp +++ b/sm.cpp @@ -29,14 +29,14 @@ along with this program. If not, see . #include "workspace.h" #include "x11client.h" #include -#include -#include #include +#include +#include "sessionadaptor.h" + namespace KWin { -static bool gs_sessionManagerIsKSMServer = false; static KConfig *sessionConfig(QString id, QString key) { static KConfig *config = nullptr; @@ -83,6 +83,8 @@ static NET::WindowType txtToWindowType(const char* txt) void Workspace::saveState(QSessionManager &sm) { + bool sessionManagerIsKSMServer = QDBusConnection::sessionBus().interface()->isServiceRegistered("org.kde.ksmserver").value(); + // If the session manager is ksmserver, save stacking // order, active window, active desktop etc. in phase 1, // as ksmserver assures no interaction will be done @@ -93,15 +95,14 @@ void Workspace::saveState(QSessionManager &sm) if (!sm.isPhase2()) { KConfigGroup cg(config, "Session"); cg.writeEntry("AllowsInteraction", sm.allowsInteraction()); - sessionSaveStarted(); - if (gs_sessionManagerIsKSMServer) // save stacking order etc. before "save file?" etc. dialogs change it + if (sessionManagerIsKSMServer) // save stacking order etc. before "save file?" etc. dialogs change it storeSession(config, SMSavePhase0); config->markAsClean(); // don't write Phase #1 data to disk sm.release(); // Qt doesn't automatically release in this case (bug?) sm.requestPhase2(); return; } - storeSession(config, gs_sessionManagerIsKSMServer ? SMSavePhase2 : SMSavePhase2Full); + storeSession(config, sessionManagerIsKSMServer ? SMSavePhase2 : SMSavePhase2Full); config->sync(); // inform the smserver on how to clean-up after us @@ -111,13 +112,6 @@ void Workspace::saveState(QSessionManager &sm) } } -// I bet this is broken, just like everywhere else in KDE -void Workspace::commitData(QSessionManager &sm) -{ - if (!sm.isPhase2()) - sessionSaveStarted(); -} - // Workspace /** @@ -361,158 +355,54 @@ SessionInfo* Workspace::takeSessionInfo(X11Client *c) return realInfo; } -// KWin's focus stealing prevention causes problems with user interaction -// during session save, as it prevents possible dialogs from getting focus. -// Therefore it's temporarily disabled during session saving. Start of -// session saving can be detected in SessionManager::saveState() above, -// but Qt doesn't have API for saying when session saved finished (either -// successfully, or was canceled). Therefore, create another connection -// to session manager, that will provide this information. -// Similarly the remember feature of window-specific settings should be disabled -// during KDE shutdown when windows may move e.g. because of Kicker going away -// (struts changing). When session saving starts, it can be cancelled, in which -// case the shutdown_cancelled callback is invoked, or it's a checkpoint that -// is immediatelly followed by save_complete, or finally it's a shutdown that -// is immediatelly followed by die callback. So getting save_yourself with shutdown -// set disables window-specific settings remembering, getting shutdown_cancelled -// re-enables, otherwise KWin will go away after die. -static void save_yourself(SmcConn conn_P, SmPointer ptr, int, Bool shutdown, int, Bool) +SessionManager::SessionManager(QObject *parent) + : QObject(parent) { - SessionSaveDoneHelper* session = reinterpret_cast< SessionSaveDoneHelper* >(ptr); - if (conn_P != session->connection()) + new SessionAdaptor(this); + QDBusConnection::sessionBus().registerObject(QStringLiteral("/Session"), this); +} + +SessionManager::~SessionManager() +{ +} + +SessionState SessionManager::state() const +{ + return m_sessionState; +} + +void SessionManager::setState(uint state) +{ + switch (state) { + case 0: + setState(SessionState::Saving); + break; + case 1: + setState(SessionState::Quitting); + break; + default: + setState(SessionState::Normal); + } +} + +void SessionManager::setState(SessionState state) +{ + if (state == m_sessionState) { return; - if (shutdown) + } + // If we're starting to save a session + if (state == SessionState::Saving) { RuleBook::self()->setUpdatesDisabled(true); - SmcSaveYourselfDone(conn_P, True); -} - -static void die(SmcConn conn_P, SmPointer ptr) -{ - SessionSaveDoneHelper* session = reinterpret_cast< SessionSaveDoneHelper* >(ptr); - if (conn_P != session->connection()) - return; - // session->saveDone(); we will quit anyway - session->close(); -} - -static void save_complete(SmcConn conn_P, SmPointer ptr) -{ - SessionSaveDoneHelper* session = reinterpret_cast< SessionSaveDoneHelper* >(ptr); - if (conn_P != session->connection()) - return; - session->saveDone(); -} - -static void shutdown_cancelled(SmcConn conn_P, SmPointer ptr) -{ - SessionSaveDoneHelper* session = reinterpret_cast< SessionSaveDoneHelper* >(ptr); - if (conn_P != session->connection()) - return; - RuleBook::self()->setUpdatesDisabled(false); // re-enable - // no need to differentiate between successful finish and cancel - session->saveDone(); -} - -void SessionSaveDoneHelper::saveDone() -{ - if (Workspace::self()) - Workspace::self()->sessionSaveDone(); -} - -SessionSaveDoneHelper::SessionSaveDoneHelper() -{ - SmcCallbacks calls; - calls.save_yourself.callback = save_yourself; - calls.save_yourself.client_data = reinterpret_cast< SmPointer >(this); - calls.die.callback = die; - calls.die.client_data = reinterpret_cast< SmPointer >(this); - calls.save_complete.callback = save_complete; - calls.save_complete.client_data = reinterpret_cast< SmPointer >(this); - calls.shutdown_cancelled.callback = shutdown_cancelled; - calls.shutdown_cancelled.client_data = reinterpret_cast< SmPointer >(this); - char* id = nullptr; - char err[ 11 ]; - conn = SmcOpenConnection(nullptr, nullptr, 1, 0, - SmcSaveYourselfProcMask | SmcDieProcMask | SmcSaveCompleteProcMask - | SmcShutdownCancelledProcMask, &calls, nullptr, &id, 10, err); - if (id != nullptr) - free(id); - if (conn == nullptr) - return; // no SM - - // detect ksmserver - char* vendor = SmcVendor(conn); - gs_sessionManagerIsKSMServer = qstrcmp(vendor, "KDE") == 0; - free(vendor); - - // set the required properties, mostly dummy values - SmPropValue propvalue[ 5 ]; - SmProp props[ 5 ]; - propvalue[ 0 ].length = sizeof(unsigned char); - unsigned char value0 = SmRestartNever; // so that this extra SM connection doesn't interfere - propvalue[ 0 ].value = &value0; - props[ 0 ].name = const_cast< char* >(SmRestartStyleHint); - props[ 0 ].type = const_cast< char* >(SmCARD8); - props[ 0 ].num_vals = 1; - props[ 0 ].vals = &propvalue[ 0 ]; - struct passwd* entry = getpwuid(geteuid()); - propvalue[ 1 ].length = entry != nullptr ? strlen(entry->pw_name) : 0; - propvalue[ 1 ].value = (SmPointer)(entry != nullptr ? entry->pw_name : ""); - props[ 1 ].name = const_cast< char* >(SmUserID); - props[ 1 ].type = const_cast< char* >(SmARRAY8); - props[ 1 ].num_vals = 1; - props[ 1 ].vals = &propvalue[ 1 ]; - propvalue[ 2 ].length = 0; - propvalue[ 2 ].value = (SmPointer)(""); - props[ 2 ].name = const_cast< char* >(SmRestartCommand); - props[ 2 ].type = const_cast< char* >(SmLISTofARRAY8); - props[ 2 ].num_vals = 1; - props[ 2 ].vals = &propvalue[ 2 ]; - propvalue[ 3 ].length = strlen("kwinsmhelper"); - propvalue[ 3 ].value = (SmPointer)"kwinsmhelper"; - props[ 3 ].name = const_cast< char* >(SmProgram); - props[ 3 ].type = const_cast< char* >(SmARRAY8); - props[ 3 ].num_vals = 1; - props[ 3 ].vals = &propvalue[ 3 ]; - propvalue[ 4 ].length = 0; - propvalue[ 4 ].value = (SmPointer)(""); - props[ 4 ].name = const_cast< char* >(SmCloneCommand); - props[ 4 ].type = const_cast< char* >(SmLISTofARRAY8); - props[ 4 ].num_vals = 1; - props[ 4 ].vals = &propvalue[ 4 ]; - SmProp* p[ 5 ] = { &props[ 0 ], &props[ 1 ], &props[ 2 ], &props[ 3 ], &props[ 4 ] }; - SmcSetProperties(conn, 5, p); - notifier = new QSocketNotifier(IceConnectionNumber(SmcGetIceConnection(conn)), - QSocketNotifier::Read, this); - connect(notifier, SIGNAL(activated(int)), SLOT(processData())); -} - -SessionSaveDoneHelper::~SessionSaveDoneHelper() -{ - close(); -} - -void SessionSaveDoneHelper::close() -{ - if (conn != nullptr) { - delete notifier; - SmcCloseConnection(conn, 0, nullptr); } - conn = nullptr; -} - -void SessionSaveDoneHelper::processData() -{ - if (conn != nullptr) - IceProcessMessages(SmcGetIceConnection(conn), nullptr, nullptr); -} - -void Workspace::sessionSaveDone() -{ - session_saving = false; - foreach (X11Client *c, clients) { - c->setSessionActivityOverride(false); + // If we're ending a save session due to either completion or cancellation + if (m_sessionState == SessionState::Saving) { + RuleBook::self()->setUpdatesDisabled(false); + Workspace::self()->forEachClient([](X11Client *client) { + client->setSessionActivityOverride(false); + }); } + m_sessionState = state; + emit stateChanged(); } } // namespace diff --git a/sm.h b/sm.h index 4cbef4bb79..7aef83516b 100644 --- a/sm.h +++ b/sm.h @@ -28,16 +28,31 @@ along with this program. If not, see . #include #include -#include -#include - -class QSocketNotifier; - namespace KWin { class X11Client; +class SessionManager : public QObject +{ + Q_OBJECT +public: + SessionManager(QObject *parent); + ~SessionManager() override; + + SessionState state() const; + +Q_SIGNALS: + void stateChanged(); + +public Q_SLOTS: // DBus API + void setState(uint state); + +private: + void setState(SessionState state); + SessionState m_sessionState; +}; + struct SessionInfo { QByteArray sessionId; QByteArray windowRole; @@ -77,25 +92,6 @@ enum SMSavePhase { SMSavePhase2Full // complete saving in phase2, there was no phase 0 }; -class KWIN_EXPORT SessionSaveDoneHelper - : public QObject -{ - Q_OBJECT -public: - SessionSaveDoneHelper(); - ~SessionSaveDoneHelper() override; - SmcConn connection() const { - return conn; - } - void saveDone(); - void close(); -private Q_SLOTS: - void processData(); -private: - QSocketNotifier* notifier; - SmcConn conn; -}; - } // namespace #endif diff --git a/workspace.cpp b/workspace.cpp index af6e838ab3..eca54f4def 100644 --- a/workspace.cpp +++ b/workspace.cpp @@ -118,7 +118,6 @@ Workspace::Workspace(const QString &sessionKey) , force_restacking(false) , showing_desktop(false) , was_user_interaction(false) - , session_saving(false) , block_focus(0) , m_userActionsMenu(new UserActionsMenu(this)) , client_keys_dialog(nullptr) @@ -128,6 +127,7 @@ Workspace::Workspace(const QString &sessionKey) , startup(nullptr) , set_active_client_recursion(0) , block_stacking_updates(0) + , m_sessionManager(new SessionManager(this)) { // If KWin was already running it saved its configuration after loosing the selection -> Reread QFuture reparseConfigFuture = QtConcurrent::run(options, &Options::reparseConfiguration); @@ -156,7 +156,6 @@ Workspace::Workspace(const QString &sessionKey) if (!sessionKey.isEmpty()) loadSessionInfo(sessionKey); - connect(qApp, &QGuiApplication::commitDataRequest, this, &Workspace::commitData); connect(qApp, &QGuiApplication::saveStateRequest, this, &Workspace::saveState); RuleBook::create(this)->load(); diff --git a/workspace.h b/workspace.h index 3c73dfd0b0..639e969d23 100644 --- a/workspace.h +++ b/workspace.h @@ -244,6 +244,8 @@ public: void stackScreenEdgesUnderOverrideRedirect(); + SessionManager *sessionManager() const; + public: QPoint cascadeOffset(const AbstractClient *c) const; @@ -339,11 +341,8 @@ public: bool globalShortcutsDisabled() const; void disableGlobalShortcutsForClient(bool disable); - void sessionSaveStarted(); - void sessionSaveDone(); void setWasUserInteraction(); bool wasUserInteraction() const; - bool sessionSaving() const; int packPositionLeft(const AbstractClient *client, int oldX, bool leftEdge) const; int packPositionRight(const AbstractClient *client, int oldX, bool rightEdge) const; @@ -492,7 +491,6 @@ private Q_SLOTS: // session management void saveState(QSessionManager &sm); - void commitData(QSessionManager &sm); Q_SIGNALS: /** @@ -616,7 +614,7 @@ private: bool was_user_interaction; QScopedPointer m_wasUserInteractionFilter; - bool session_saving; + int session_active_client; int session_desktop; @@ -666,6 +664,7 @@ private: QList m_genericEventFilters; QScopedPointer m_movingClientFilter; + SessionManager *m_sessionManager; private: friend bool performTransiencyCheck(); friend Workspace *workspace(); @@ -742,14 +741,9 @@ inline bool Workspace::wasUserInteraction() const return was_user_interaction; } -inline void Workspace::sessionSaveStarted() +inline SessionManager *Workspace::sessionManager() const { - session_saving = true; -} - -inline bool Workspace::sessionSaving() const -{ - return session_saving; + return m_sessionManager; } inline bool Workspace::showingDesktop() const