[ksmserver] Rewrite Startup
ClosedPublic

Authored by davidedmundson on Oct 15 2018, 7:24 PM.

Details

Summary

The code is effectively 3 startup phases each of which has some parallel
methods that need to be complete before moving onto the next phase.

The old code had a tonne of methods all of which tracking what state
we're in and trying to start the next method in the chain handling
things out of order.

This simplifies everything into 3 composite kjobs. A lot more classes,
but each one tiny with the flow more readable.

Code should be effectively the same. The startup sound is moved ever so
slightly earlier to be when phase 2 starts rather than just after
kcminit2 finishes (midway through the old phase 2), but no significant
behavioural changes here.

Test Plan

Logged in with session restore and without

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3916
Build 3934: arc lint + arc unit
davidedmundson created this revision.Oct 15 2018, 7:24 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 15 2018, 7:24 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Oct 15 2018, 7:24 PM
anthonyfieroni added inline comments.
ksmserver/startup.cpp
194

Can we delay it to finishStartup?

210

It's unused, can we remove it?

ksmserver/startup.h
39

Remove ?

davidedmundson marked an inline comment as done.Oct 15 2018, 11:20 PM
davidedmundson added inline comments.
ksmserver/startup.cpp
194

Probably, but for now I'm trying to be as close to 1:1 with the old code.

Long term plan ideally I want these just in a .desktop file that kglobalaccel can use directly.

210

it's called from main

Pretty cool!

ksmserver/startup.cpp
94–95

Can these happen in parallel? In the old code (as far as I can understand it) it did auto start first and then kcminit phase 1? or does addSubjob queue them one after the other (doesn't look like it)?

143

As a next step I'd like to see this ported to libcanberra :P

206

Coding style, }); on new line

ksmserver/startup.cpp
94–95

Well read!

It was deliberate, but I should have commented it in the description.

Autostart apps job only launches, it doesn't (currently) block waiting for them to start.

Therefore it's effectively in parallel already.

143

Canberra has a systemd unit that plays startup/shutdown sounds automatically. It could be entirely someone elses problem. \o/

apol accepted this revision.Oct 30 2018, 3:49 PM
apol added a subscriber: apol.

+1 LGTM

ksmserver/startup.cpp
275

can't this be &KJob::emitResult?

This revision is now accepted and ready to land.Oct 30 2018, 3:49 PM
davidedmundson added inline comments.Oct 30 2018, 3:56 PM
ksmserver/startup.cpp
275

It's stupid:

I can do

connect(watch, SIGNAL(finished()), this, SLOT(emitResult()));

but not with function pointers because:

error: 'emitResult' is a protected member of 'KJob'

connect(watcher, &QDBusPendingCallWatcher::finished, this, &KJob::emitResult);
This revision was automatically updated to reflect the committed changes.