graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.
Overall I would also like to see autotests for it similar to the tests for
other interfaces.
INLINE COMMENTS
> remote_access_interface.cpp:146-149
> + if (bound) {
> + wl_client_post_no_memory(client);
> + return;
> + }
if really only one client should be allowed (why?) it would be better to send a
dedicated error state to inform it instead of "abusing" no memory.
> remote_access_interface.cpp:158
> + wl_resource_set_implementation(resource, &s_interface, this, unbind);
> + m_resource = resource;
> + bound = true;
this allows to have only one client bind it. As soon as a second client binds
the protocol it will get overwritten and breaks the existing one.
I think you need a QVector<wl_resource*> here.
> remote_access_interface.h:43
> + **/
> +struct GbmBuffer
> +{
for ABI compatibility we cannot have structs defined in the header. See
https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts
for explanation. Especially the last point of the Donts is relevant as it
means we are never able to change this again.
Thus I think the proper way has to be:
class Buffer
{
public:
void setFd(int qint32);
void setSize(const QSize &size);
void setStride(qint32 stride);
enum class Format {
Format1,
Format2
};
void setFormat(Format format);
private:
class Private;
QScopedPointer<Private> d;
};
and then in the Implementation have the Private class which holds the data
members.
> remote_access_interface.h:94-112
> +class KWAYLANDSERVER_EXPORT RemoteBufferInterface : public Resource
> +{
> + Q_OBJECT
> +public:
> + virtual ~RemoteBufferInterface() = default;
> +
> + /**
I don't see this class exposed in any way in the API. So a user of the library
cannot have access to it.
Thus I suggest to move it into a private header and remove the
KWAYLANDSERVER_EXPORT. It's a private class to the library then, which makes
life easier.
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
