[Differential] [Updated] D1231: Add Remote Access interface to KWayland

[Kanedias (Oleg Chernovskiy)]

Kanedias marked 8 inline comments as done.
Kanedias added inline comments.

INLINE COMMENTS

> graesslin wrote in remote-access.xml:19
> I noticed that standard Wayland protocols also have a destructor request for the manager interface. I'd suggest to also add it (it makes sense as then the server can destroy the resource).

Added destructor and release callback.

> graesslin wrote in remote-access.xml:33
> so the idea here would be that if in future we want to support other buffers (e.g. shared mem) we just add a new event?

Yes, with appropriate "since" clause

> graesslin wrote in remote-access.xml:41
> Just for thought: in other interfaces the destructor is mostly called "release"

Renamed

> graesslin wrote in registry.h:128
> please add as last interface otherwise it breaks API

Moved to last in enum. Or did you mean in forward references declaration too?..

> graesslin wrote in registry.h:379
> we moved to frameworks which means we are now at 5.23 - sorry about that. I also had to rename a bunch of @since 5.7 ;-)

Corrected

> graesslin wrote in remote_access_interface.cpp:146-149
> 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.

Added ability to have multiple clients in the same time

> graesslin wrote in remote_access_interface.cpp:158
> 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.

Reimplemented

> graesslin wrote in remote_access_interface.h:46
> For ABI compatibility it's better to only have d-ptr-ized classes/structs in the public header.

Implemented GbmBuffer with d-pointer

> graesslin wrote in remote_access_interface.h:49-50
> why do we need gbm_surface and gbm_bo? This looks like adding not needed details to the interface

Removed, reimplemented

> graesslin wrote in remote_access_interface.h:43
> 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.

Thanks for clarification.
Reimplemented.

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/20160611/72cd25fb/attachment-0001.html>