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
