D13510: Add XDG WM Base support to our XDGShell API

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> test_xdg_shell.cpp:120
> +        break;
> +    }
>  

Missing `default`. Is it okay if version is unknown?

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

Seems like QDebug is not used here.

> xdgshell_stable.cpp:307
> +    Q_UNUSED(surface)
> +    auto s = reinterpret_cast<Private*>(data);
> +    s->q->configureRequested(s->pendingSize, s->pendingState, serial);

Use static_cast. Same down below.

> xdgshell_stable.cpp:336
> +            states = states | XdgShellSurface::State::Activated;
> +            break;
> +        }

Missing `default`. Is it okay if the state is unknown?

> xdgshell_stable.cpp:525
> +
> +const struct xdg_popup_listener XdgShellPopupStable::Private::s_popupListener = {
> +    configureCallback,

Shouldn't it be also static?

> xdgshell_stable.cpp:536
> +{
> +    Q_UNUSED(xdg_popup);
> +    auto s = reinterpret_cast<Private*>(data);

Nitpick: no semicolons after Q_UNUSED.

> xdgshell_stable_interface.cpp:75
> +    Private(XdgPopupStableInterface *q, XdgShellStableInterface *c, SurfaceInterface *surface, wl_resource *parentResource);
> +    ~Private();
> +

I would also add override, e.g.

  ~Private() override;

> xdgshell_stable_interface.cpp:242-243
> +        [surface](XdgSurfaceStableInterface *s) {
> +            return false;
> +            return surface == s->surface();
> +        }

Which one should return?

> xdgshell_stable_interface.cpp:488-493
> +    if (parentXdgSurface) {
> +        pd->parent = parentXdgSurface->surface();
> +    } else {
> +        wl_resource_post_error(parentResource, XDG_WM_BASE_ERROR_INVALID_POPUP_PARENT, "Invalid popup parent");
> +        return;
> +    }

Can be simplified, e.g.

  if (!parentXdgSurface) {
      wl_resource_post_error(parentResource, XDG_WM_BASE_ERROR_INVALID_POPUP_PARENT, "Invalid popup parent");
      return;
  }
  
  pd->parent = parentXdgSurface->surface();
  pd->....

REPOSITORY
  R127 KWayland

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

To: davidedmundson, #kwin
Cc: zzag, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180613/5ced4dc6/attachment-0001.html>