D6047: Support XDG v6

[Martin Flöser]

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


  First round of review, client api is done.

INLINE COMMENTS

> registry.h:1275
> +     * @param name The name for the removed interface
> +     * @since 5.25
> +     **/

careful here: the since won't match

> xdgshell.h:95
> +
> +    /*
> +     * Which edge of the anchor should the popup be positioned around

In case you wanted to make those comments doxygen comments: you need to use /** instead of /*

Applies for all comments in this class.

> xdgshell.h:176
> +
> +    void setup(zxdg_shell_v6 *xdgshellv6);
> +

lacking documentation including an @since.

> xdgshell.h:222
>       **/
>      XdgShellPopup *createPopup(Surface *surface, Surface *parentSurface, Seat *seat, quint32 serial, const QPoint &parentPos, QObject *parent = nullptr);
>  

If the comment is deprecated, the method should be deprecated as well. But I disagree on the point of deprecation. It is not deprecated if it's used with xdgShellv5. That should probably just be documented properly.

> xdgshell.h:226
> +     * Creates a new XdgShellPopup for the given @p surface on top of @p parentSurface with the given @p positioner.
> +     **/
> +    XdgShellPopup *createPopup(Surface *surface, XdgShellSurface *parentSurface, const XdgPositioner &positioner, QObject *parent = nullptr);

@since missing

> xdgshell.h:229
> +
> +        /**
> +     * Creates a new XdgShellPopup for the given @p surface on top of @p parentSurface with the given @p positioner.

looks like wrong indentation.

> xdgshell.h:231
> +     * Creates a new XdgShellPopup for the given @p surface on top of @p parentSurface with the given @p positioner.
> +     **/
> +    XdgShellPopup *createPopup(Surface *surface, XdgShellPopup *parentSurface, const XdgPositioner &positioner, QObject *parent = nullptr);

@since missing

> xdgshell.h:302
> +
> +    void setup(zxdg_surface_v6 *xdgsurfacev6, zxdg_toplevel_v6 *toplevel);
> +

missing documentation.

> xdgshell.h:427
>  
> +    /*
> +     * Set this surface to have a given maximum size

same as with the other class: that's not a documentation comment.

> xdgshell.h:500
> +     * method.
> +     **/
> +    void setup(zxdg_surface_v6 *xdgsurfacev6, zxdg_popup_v6 *xdgpopup6);

@since missing

> xdgshell.h:562
> +    /**
> +     *
> +     **/

documentation missing

> xdgshell_p.h:67
> +        Q_UNUSED(parent)
> +        qWarning() << __func__ << " is not supported in this version";
> +        return nullptr;

Please no qWarning. Either qCWarning or not at all.

> xdgshell_v6.cpp:28
> +
> +#include <QDebug>
> +

I don't see any debug messages in this file

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #plasma, graesslin
Cc: graesslin, mart, plasma-devel, #frameworks, leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, hein, lukas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170825/42017eef/attachment-0001.html>