[Differential] [Updated] D1989: Introduce QQuickItem to nest kwin_wayland

[graesslin (Martin Gräßlin)]

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