graesslin added a comment.
I wouldn't put the kwinqml into the qml subdirectory. As it's a plugin I
think the plugins directory is a better target. Maybe
plugins/qtquick/kwinwrapper - not sure about the last part of the path. If you
have better ideas, go for it.
INLINE COMMENTS
> kwinqml.cpp:22-23
> +{
> +}
> +void KWinQml::start()
> +{
empty line please
> kwinqml.cpp:44
> + if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sx) < 0) {
> + QCoreApplication::instance()->exit(1);
> + return;
I wouldn't terminate the application from the QtQuick module. That must be more
graceful.
> kwinqml.cpp:50
> + if (socket == -1) {
> + QCoreApplication::instance()->exit(1);
> + return;
same here
> kwinqml.cpp:56
> + QProcessEnvironment environment =
> QProcessEnvironment::systemEnvironment();
> + environment.insert(QStringLiteral("QT_QPA_PLATFORM"),
> QStringLiteral("wayland"));
> + environment.insert(QStringLiteral("WAYLAND_SOCKET"),
> QString::fromUtf8(QByteArray::number(socket)));
no need to set the QT_QPA_PLATFORM, kwin overrides it anyway.
> kwinqml.cpp:62
> + arguments << "--xwayland";
> + kwinWayland->start("kwin_wayland", arguments);
> +}
we should try to get the install path of kwin_wayland here to make sure our
binary is started and not some PATH overwritten one. An example of how to do
this is in https://phabricator.kde.org/D1973 with the difference that
kwin_wayland is installed to bin instead of libexec
> kwinqml.h:17
> + Q_OBJECT
> + Q_PROPERTY(QString socketName READ socketName WRITE setSocketName NOTIFY
> socketNameChanged)
> +
what's the idea behind the socketName? It's not yet used anywhere.
> kwinqml.h:23-27
> + void setSocketName(const QString socketName)
> + {
> + m_name = "kwin-plasma-emulator-0";
> + emit socketNameChanged(socketName);
> + }
this doesn't set the m_name to socketName
> kwinqml.h:34
> +
> + Q_INVOKABLE void start();
> +
instead of using start which needs to be invoked manually, you could also react
on the initialization completed in the QQuickItem
> kwinqml.h:40
> +private:
> + QString m_name;
> + KWayland::Server::Display *m_display = nullptr;
why m_name and not m_socketName?
REPOSITORY
rKWIN KWin
REVISION DETAIL
https://phabricator.kde.org/D1989
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: bdhruve, #plasma_on_wayland
Cc: graesslin, plasma-devel, kwin, hardening, jensreuterberg, sebas
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
