broulik added inline comments.
INLINE COMMENTS
> fvogt wrote in startplasma-wayland.cpp:41
> AFAICT this won't work as expected: localed is only launched on demand, which
> `isServiceRegistered` does not care about.
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()`
> startplasma-wayland.cpp:43
> + auto queryAndSet = [](const QByteArray &var, const QByteArray &
> value) {
> + QDBusInterface iface(QStringLiteral("org.freedesktop.locale1"),
> QStringLiteral("/org/freedesktop/locale1"),
> QStringLiteral("org.freedesktop.locale1"), QDBusConnection::systemBus());
> +
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.
> startplasma-x11.cpp:78
> + qputenv("GDK_SCALE", screenScaleFactors);
> + qputenv("GDK_DPI_SCALE",
> QByteArray::number(1/screenScaleFactors.toInt(), 'g', 3));
> + }
Won't that do an integer division?
> startplasma.cpp:82
> + QStringList filteredFiles;
> + std::copy_if(files.begin(), files.end(),
> std::back_inserter(filteredFiles), [](const QString& i){ return
> QFileInfo(i).isReadable(); } );
> +
Should we also check for executable permission?
> startplasma.cpp:241
> +
> + runSync(QStringLiteral("xsetroot"), {QStringLiteral("-cursor_name"),
> QStringLiteral("left_ptr")});
> + runSync(QStringLiteral("xprop"), {QStringLiteral("-root"),
> QStringLiteral("-f"), QStringLiteral("KDE_FULL_SESSION"),
> QStringLiteral("8t"), QStringLiteral("-set"),
> QStringLiteral("KDE_FULL_SESSION"), QStringLiteral("true")});
Would it make a difference using libxcb for this instead of spawning an
external process synchronously here?
> startplasma.cpp:293
> +
> +static bool dl = false;
> +
descriptive variable name, please.
> startplasma.cpp:308
> + p = new QProcess;
> + p->start(QStringLiteral("ksplashqml"), {
> ksplashCfg.readEntry("Theme", QStringLiteral("Breeze")) });
> + }
KSplash internally forks and prints the PID on stdout which doesn't seem
neccessary anymore with this new code. (Optimization for the future maybe)
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D21725
To: apol, #plasma, fvogt
Cc: broulik, fvogt, davidedmundson, plasma-devel, LeGast00n, ericadams,
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
