Port one of session management connections state to a custom API
ClosedPublic

Authored by davidedmundson on Oct 22 2019, 12:45 PM.

Details

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.

Diff Detail

Repository
R108 KWin
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18407
Build 18425: arc lint + arc unit
Restricted Application added a project: KWin. · View Herald TranscriptOct 22 2019, 12:45 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Oct 22 2019, 12:45 PM

sneaky update

zzag added a subscriber: zzag.Oct 22 2019, 2:25 PM

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.

sm.cpp
366

Style: Add whitespace between colon and QObject

404–405

Wrap raw c strings in QStringLiteral.

sm.h
30–31

Unused forward declaration.

94–95

Style: Put whitespace before colon.

workspace.cpp
1834

Put whitespace between // and the text.

1841
  • Don't use foreach in new code.
  • Change c to client
workspace.h
481–486

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?

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 anythin

That's my longer term plan :D : https://phabricator.kde.org/T11882
I had to split it as the ksmserver side got a bit mental (but in a good way)

workspace.cpp
1834

I can't wait for clang-format!

workspace.h
481–486

Yeah, I was torn originally, so no objections.

My reasoning against was that I didn't want to have the state in one place and Workspace::sessionSaving() in another, but I could port all the relevant code calling that.

zzag added inline comments.Oct 22 2019, 4:34 PM
workspace.cpp
1834

Unfortunately clang-format doesn't format comments.

workspace.h
481–486

Yes please

davidedmundson marked 11 inline comments as done.

Make SessionManager class the authority of session state
+ other review comments

zzag added inline comments.Oct 24 2019, 12:34 PM
sm.cpp
98

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

sm.h
36

Style: Put whitespace between SessionManager and :.

41

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

workspace.h
744

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.

sm.cpp
98

That is true right now, but I have intentions to port save and half porting part of that now only complicates things - especially if we do keep an XSM path.

davidedmundson marked 3 inline comments as done.Oct 24 2019, 1:10 PM
davidedmundson added inline comments.
sm.cpp
98

I'll add a

gs_sessionManagerIsKSMServer = getenv(session) == "plasma"

That way we don't even have a micro regression if I don't finish the rest before next release.

Nice, replacing SessionSaveDoneHelper with the class. I assume the change on Ksmserver to call into KWin alreadyfor setting the state landed or how does this work?

zzag accepted this revision.Oct 25 2019, 9:11 PM

Some remaining nitpicks.

org.kde.KWin.Session.xml
6–9

Indentation is off.

sm.cpp
372

Use QStringLiteral please.

workspace.h
247

Style: Put whitespace before *.

744

This inline comment is not addressed.

This revision is now accepted and ready to land.Oct 25 2019, 9:11 PM
davidedmundson marked an inline comment as done.

update to ksmserver changes

apol accepted this revision.Nov 1 2019, 1:37 AM
zzag added inline comments.Nov 1 2019, 10:11 AM
sm.cpp
377–384

Style: Case labels should not be indented.


Also, I'd prefer to be more explicit about the conversion between "d-bus enums" and our enums, i.e. use integer values in the case labels.

Another way to implement this method without encoding "d-bus enum" values in our types is to use a static QVector and value() method.

I don't have a strong opinion though. It's up to you whether to leave this method as is. However, please fix the coding style issue.

zzag added inline comments.Nov 1 2019, 10:12 AM
sm.cpp
377–384

s/integer values/integer literals/

This revision was automatically updated to reflect the committed changes.