D21725: Use a native application for starting plasma

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190611/92ad2749/attachment-0001.html>