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
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
