D7369: Wayland foreign protocol

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  Overall lots of documentation is missing.
  
  From API layout that looks much better now, but we still have a few places where "Unstable" is needlessly in the API. Most important in Display the create method is not forward compatible by not having an enum to describe which interface version should be created.

INLINE COMMENTS

> registry.h:527-528
> +
> +    zxdg_exporter_v1 *bindXdgExporterUnstableV1(uint32_t name, uint32_t version) const;
> +    zxdg_importer_v1 *bindXdgImporterUnstableV1(uint32_t name, uint32_t version) const;
>      ///@}

please add documentation

> registry.h:940-941
> +
> +    XdgExporter *createXdgExporterUnstable(quint32 name, quint32 version, QObject *parent = nullptr);
> +    XdgImporter *createXdgImporterUnstable(quint32 name, quint32 version, QObject *parent = nullptr);
>      ///@}

please add documentation

> registry.h:940-941
> +
> +    XdgExporter *createXdgExporterUnstable(quint32 name, quint32 version, QObject *parent = nullptr);
> +    XdgImporter *createXdgImporterUnstable(quint32 name, quint32 version, QObject *parent = nullptr);
>      ///@}

Why did you keep the suffix "Unstable"?

> registry.h:1132-1133
> +
> +    void exporterUnstableV1Announced(quint32 name, quint32 version);
> +    void importerUnstableV1Announced(quint32 name, quint32 version);
>      ///@}

please add documentation

> registry.h:1288-1290
> +    void exporterUnstableV1Removed(quint32 name);
> +    void importerUnstableV1Removed(quint32 name);
>      ///@}

please add documentation

> display.h:223
>  
> +    XdgForeignInterface *createXdgForeignUnstableInterface(QObject *parent = nullptr);
>      /**

please add documentation.

Also the common way would be to have an enum to describe which interface should be created, but therefore drop the Unstable from the API name.

> xdgforeign_interface.h:37
> +class XdgImporterUnstableV1Interface;
> +
> +class KWAYLANDSERVER_EXPORT XdgForeignInterface : public QObject

Something like an enum ForeignInterfaceVersion is missing, please compare TextInputInterface

> xdgforeign_v1_interface.cpp:260
> +
> +    QPointer<XdgImportedUnstableV1Interface> imp = new XdgImportedUnstableV1Interface(s->q, surface);
> +    imp->create(s->display->getConnection(client), wl_resource_get_version(resource), id);

what's an imp? Could you please write full variable names?

REPOSITORY
  R127 KWayland

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

To: mart, #plasma, #kwin, davidedmundson, graesslin
Cc: davidedmundson, graesslin, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170905/61891cdc/attachment-0001.html>