D24862: Port one of session management connections state to a custom API

zzag added a comment.


  Sweet. I like the SessionManager class. I wonder though whether we could put more responsibility on it, e.g. loading and storing session. I don't ask you to implement that or anything. I'm just trying to digest the idea of having a class responsible for session management and what potential code improvements/simplifications the class brings with itself.
  
  Some coding style nitpicks.

INLINE COMMENTS

> sm.cpp:358
> +SessionManager::SessionManager(QObject *parent)
> +    :QObject(parent)
>  {

Style: Add whitespace between colon and QObject

> sm.cpp:371-372
> +    static QHash<QString, SessionState> s_stateMap = {
> +        {"saving", SessionState::Saving},
> +        {"quitting", SessionState::Quitting}
> +    };

Wrap raw c strings in QStringLiteral.

> sm.h:31
>  
> -#include <X11/SM/SMlib.h>
> -#include <fixx11h.h>
> -
>  class QSocketNotifier;
>  

Unused forward declaration.

> sm.h:77
>  
> -class KWIN_EXPORT SessionSaveDoneHelper
> -    : public QObject
> +class SessionManager: public QObject
>  {

Style: Put whitespace before colon.

> workspace.cpp:1834
> +    }
> +    //starting session save
> +    if (state == SessionState::Saving) {

Put whitespace between `//` and the text.

> workspace.cpp:1841
> +        RuleBook::self()->setUpdatesDisabled(false);
> +        foreach (X11Client *c, clients) {
> +            c->setSessionActivityOverride(false);

- Don't use foreach in new code.
- Change `c` to `client`

> workspace.h:480-485
> +    /**
> +     * Updates kwin's internal knowledge of the state of the
> +     * session outside.
> +     */
> +    void setSessionState(SessionState state);
> +    SessionState sessionState() const;

Hmm, that's kinda domain of SessionManager. Also, the last one doesn't qualify as a slot.

What do you think about moving these two to SessionManager?

REPOSITORY
  R108 KWin

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

To: davidedmundson, #kwin
Cc: zzag, kwin, LeGast00n, The-Feren-OS-Dev, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart