Use a native application for starting plasma
ClosedPublic

Authored by apol on Jun 10 2019, 3:23 PM.

Details

Reviewers
fvogt
davidedmundson
Group Reviewers
Plasma
Maniphest Tasks
T10958: Faster Startup
Commits
R120:cfd6685b7e46: Fix passing extra envs to runSync
R120:3bbe484baa73: Fix starting locked
R120:f92b72f126e4: right configuration file for cursors
R120:8d6413e3978f: Use a native application for starting plasma
R120:f0647b712c52: Use a native application for starting plasma
R120:319ea22434da: Add warning when processes don't succeed
R120:95ee6c4b0ff5: Fix env var incorporation
R120:2868abdfcf10: Remove unused file
R120:158cbf3693fc: first
R120:9decafa3d153: Can't know things change if they are not exported
R120:cfa2f0f8617d: Can't know things change if they are not exported
R120:5e29825c8136: Remove unused code
R120:8569573a2075: Set defaults
R120:8c4fdfd1e807: Port approach to wayland
R120:0d2a35052263: Add missing files
R120:13848110a747: Fix passing extra envs to runSync
R120:47b666ddc768: Remove unused code
R120:676924a79aa3: Don't go through an external source to read a config file
R120:07bd92ccc061: Don't forward sourced environment variables that describe the helper process
R120:4d7322701737: Set defaults
R120:bde0e9f68511: right configuration file for cursors
R120:dbc166852731: Fixes
R120:dc3055e2260e: Port approach to wayland
R120:92ad8096ac90: Add warning when processes don't succeed
R120:a470a952bc02: Don't go through an external source to read a config file
R120:5b13ff50b6b1: first approach to porting the startkde sh into a native process
R120:24853f46560a: Use a native application for starting plasma
R120:724890a55c05: Don't forward sourced environment variables that describe the helper process
Summary

At the moment we had several scripts to start the different processes. With this we unify this code into an application that takes care of the whole process.
This allows us to:

  • Save on process spawning, we don't need to run a separate process synchronously for every single thing.
  • Don't have a redundant configuration file parser but reuse the one we've already optimised in KConfig.
  • Issue dbus calls from the process itself instead of spawning qdbus.
  • Removes a bunch of duplicated code.
Test Plan

Started different systems on:

archlinux: wayland and x11
neon: wayland and x11

In general, the logic here is 1:1 to what the script used to do and when there's been issues it was within the port and it was easily addressed.

Diff Detail

Repository
R120 Plasma Workspace
Branch
apol/startkde
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12792
Build 12810: arc lint + arc unit
apol created this revision.Jun 10 2019, 3:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 10 2019, 3:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Jun 10 2019, 3:23 PM

Concept ++

Started different systems on different distros, on wayland and x11.

Given this is a huge change, please expand on this and confirm we've covered all paths.

Debug needs cleaning (or QCDebug) before merge.


#preload the user's locale on first start

is missing


startkde/startplasma.cpp
143

Don't yet.

This being here is (I think) the result of a lazy hack due to env vars not getting synced properly from kcminit. I hope to address that.

Obviously your port being a 1:1 clone is fine.

259

braces

apol edited the test plan for this revision. (Show Details)Jun 10 2019, 4:12 PM
apol updated this revision to Diff 59525.Jun 10 2019, 4:30 PM

Make sure plasma-localerc is created

apol updated this revision to Diff 59528.Jun 10 2019, 4:34 PM

Clean debug

apol marked an inline comment as done.Jun 10 2019, 4:36 PM
apol added inline comments.
startkde/startplasma.cpp
143

I won't, yet.

apol updated this revision to Diff 59529.Jun 10 2019, 4:37 PM

braces

fvogt requested changes to this revision.Jun 10 2019, 5:28 PM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
startkde/startplasma-wayland.cpp
42

AFAICT this won't work as expected: localed is only launched on demand, which isServiceRegistered does not care about.

This revision now requires changes to proceed.Jun 10 2019, 5:28 PM
broulik added inline comments.
startkde/startplasma-wayland.cpp
42

Indeed. When it's not registered, query

"org.freedesktop.DBus",
"/org/freedesktop/DBus",
"org.freedesktop.DBus",
"ListActivatableNames"

(unfortunately no Qt API for that) and then call startService()

44

Please use a DBus-interface generated from XML instead of QDBusInterface to avoid doing a blocking introspection call.
You also might want to move the object outside the lamda so it's not recreated for every call you do below.

startkde/startplasma-x11.cpp
79

Won't that do an integer division?

startkde/startplasma.cpp
83

Should we also check for executable permission?

242

Would it make a difference using libxcb for this instead of spawning an external process synchronously here?

294

descriptive variable name, please.

309

KSplash internally forks and prints the PID on stdout which doesn't seem neccessary anymore with this new code. (Optimization for the future maybe)

davidedmundson added inline comments.Jun 11 2019, 8:28 AM
startkde/startplasma-wayland.cpp
42

Generally it's easier and faster to just call the method you want to call and see if it fails.

apol marked 6 inline comments as done.Jun 11 2019, 2:46 PM
apol added inline comments.
startkde/startplasma.cpp
83

We don't now and it's not required. I'd aim at not changes in behaviour.

242

I wonder so as well. Possibly. It would also make the process link against libxcb.
Something to try I'd say.

Maybe once it's in.

apol updated this revision to Diff 59597.Jun 11 2019, 2:46 PM

addressed suggested changes

apol updated this revision to Diff 59693.Jun 12 2019, 9:38 PM

--verbose

Two minor things and then this is good to go.

startkde/startplasma.cpp
35

old code killed ksplash when we showed a message

124

Not a blocker, but why not use KConfig here?

285

This is wrong.

forceFontDPI is a number
we need to read that from the config file and send that to xrdb.

you can verify it works with

xrdb -q | grep Xft

in your session afterwards

apol marked an inline comment as done.Jun 13 2019, 7:52 PM
apol added inline comments.
startkde/startplasma.cpp
35

Now we will return, which will get KSplash out of scope and kill the process upon destruction. See KillBeforeDeleter.

124

Because this is clearly faster and not complex at all to read. I can change it if you like.
This way we don't worry about reading global files and such.

285

Good catch.

apol updated this revision to Diff 59752.Jun 13 2019, 7:53 PM
apol marked an inline comment as done.

Address xrdb call as suggested by David

davidedmundson accepted this revision.Jun 13 2019, 8:49 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2019, 2:17 PM
This revision was automatically updated to reflect the committed changes.