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

Vlad Zahorodnii noreply at phabricator.kde.org
Thu Oct 24 13:34:52 BST 2019


zzag added inline comments.

INLINE COMMENTS

> sm.cpp:97
>          cg.writeEntry("AllowsInteraction", sm.allowsInteraction());
> -        sessionSaveStarted();
>          if (gs_sessionManagerIsKSMServer)   // save stacking order etc. before "save file?" etc. dialogs change it
>              storeSession(config, SMSavePhase0);

Hmm, we no longer enter this path since gs_sessionManagerIsKSMServer is always false.

> sm.h:36
>  
> +class SessionManager: public QObject
> +{

Style: Put whitespace between `SessionManager` and `:`.

> sm.h:41
> +    SessionManager(QObject *parent);
> +    ~SessionManager();
> +

Nitpick: In order to be consistent with other code, add `override` keyword.

> workspace.h:743
>  
> -inline void Workspace::sessionSaveStarted()
> -{
> -    session_saving = true;
> -}
> -
> -inline bool Workspace::sessionSaving() const
> +inline SessionManager* Workspace::sessionManager() const
>  {

Style: We still follow the Frameworks coding style, which says that we have to put whitespace before `*`.

One could argue that return types have special meaning, but a coding review is not a place to enforce personal preferences that are uncommon or haven't been agreed upon by other developers. Such matter must be raised to the discussion in other places, e.g. a task or a mailing list. Also, no such a tool exists that would format code with this style.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20191024/1bedef71/attachment.html>


More information about the kwin mailing list