[Differential] [Commented On] D1231: Add Remote Access interface to KWayland

[graesslin (Martin Gräßlin)]

graesslin added inline comments.

INLINE COMMENTS

> test_remote_access.cpp:161
> +    // client fd is different, not subject to check
> +    QVERIFY(rbuf->width() == 50);
> +    QVERIFY(rbuf->height() == 50);

use QCOMPARE to test two values. If it fails with QCOMPARE you get the values it actually had. With QVERIFY you only see that it failed

> registry.h:144
> +        TextInputManagerUnstableV2, ///< Refers to zwp_text_input_manager_v2, @since 5.23
> +        RemoteAccessManager ///< Refers to org_kde_kwin_remote_access_manager interface, @since 5.23
>      };

just a note: it's 5.24 since today...

> remote_access.cpp:66
> +    rbuf->setup(requested);
> +    qDebug() << "Client: Got buffer, server fd:" << buffer_id;
> +

qCDebug please

> remote_access.h:193
> +Q_SIGNALS:
> +    void paramsObtained();
> +

hmm maybe parametersObtained?

> remote_access_interface.cpp:28
> +
> +#include <QtCore/QDebug>
> +#include <QHash>

you can just include "logging_p.h"

> remote_access_interface.cpp:240
> +
> +        qDebug() << "Server: remote buffer returned, client" << wl_resource_get_id(resource)
> +                                                     << ", id" << rbuf->id()

qCDebug

> remote_access_interface.cpp:263
> +        // no more clients using this buffer
> +        qDebug() << "Server: buffer released, fd" << bh.buf->fd();
> +        emit q->bufferReleased(bh.buf);

qCDebug

> remote_access_interface.cpp:336
> +const struct org_kde_kwin_remote_buffer_interface RemoteBufferInterface::Private::s_interface = {
> +    releaseCallback
> +};

you can use the new resourceDestroyedCallback in resource_p.h. It handles the destroy correctly and that will trigger the unbind and deleteLater automatically.

> remote_access_interface.cpp:346
> +
> +    p->q->deleteLater(); // also purges it from manager's list
> +}

that would trigger a double delete as I had to learn very painfully lately.

> remote_access_interface.cpp:356-359
> +    if (resource) {
> +        wl_resource_destroy(resource);
> +        resource = nullptr;
> +    }

you don't need that, it's already in Resource

> remote_access_interface.h:45
> + **/
> +class KWAYLANDSERVER_EXPORT GbmBuffer
> +{

Thinking out loud:

What if in future we pass non GbmBuffers? Should we then still call it GbmBuffer or do we need a new class? Maybe we should make it a nested class to RemoteAccessManagerInterface and then just call it Buffer? Or keep it outside and call it more generic RemoteBuffer?

> remote_access_interface.h:77
> +     **/
> +    void sendBufferReady(const GbmBuffer *buf);
> +    /**

who's the owner of the GbmBuffer? Who will delete it?

> remote_access_interface_p.h:33
> + * @brief Holds data of passed buffer and client resource. Also controls buffer lifecycle.
> + */
> +class RemoteBufferInterface : public Resource

could you please add a note about that it's an internal class

> remote_access_interface_p.h:58
> +#endif
> \ No newline at end of file


nitpick

REPOSITORY
  rKWAYLAND KWayland

REVISION DETAIL
  https://phabricator.kde.org/D1231

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: Kanedias, graesslin
Cc: plasma-devel, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160613/0b29f63f/attachment-0001.html>