[Differential] [Requested Changes To] D1231: Add Krfb interface to KWayland

[graesslin (Martin Gräßlin)]

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