zzag added inline comments.
INLINE COMMENTS
> display.cpp:49
> +#include "inputmethod_interface.h"
> +
>
Stray new line. Please remove it.
> inputmethod_interface.cpp:25
> +
> +class InputMethodContextInterface::Private : public
> QtWaylandServer::zwp_input_method_context_v1
> +{
Add Q_DECL_HIDDEN.
> inputmethod_interface.cpp:125
> +
> + void
> zwp_input_method_context_v1_destroy(QtWaylandServer::zwp_input_method_context_v1::Resource
> *resource) override
> + {
In the destructor request, we need to destroy the wl_resource with
`wl_resource_destroy()`. When the resource is destroyed and it isn't inert,
destroy `q`.
---
We probably need to destroy the wl_resource resource here.
wl_resource_destroy(resource->handle);
> inputmethod_interface.cpp:145-146
> +{
> + for (auto r : d->resourceMap())
> + d->send_commit_state(r->handle, serial);
> +}
I also prefer not to put braces around for's and if's when the body contains
only one statement. Code looks more compact; but we follow the Frameworks
coding style and should put braces even if it's a single statement.
> inputmethod_interface.cpp:179
> +
> +class InputPanelSurfaceInterface::Private : public
> QtWaylandServer::zwp_input_panel_surface_v1
> +{
Shouldn't `InputPanelSurfaceInterface` be also a subclass of `SurfaceRole`?
> inputmethod_interface.cpp:199-200
> + InputPanelSurfaceInterface *const q;
> + QPointer<SurfaceInterface> m_surface;
> + bool m_overlay = false;
> +};
Naming nitpick: in `FooPrivate` classes, we avoid putting `m_`.
> inputmethod_interface.cpp:220
> +
> + void zwp_input_panel_v1_get_input_panel_surface(Resource *resource,
> uint32_t id, struct ::wl_resource *surface) override
> + {
Naming nitpick: it would be nice to avoid names such as `surfaceIface` or
`surfaceInterface`. I would like to highlight that `surfaceIface` is a bad name
according to the Frameworks coding style.
Suggestion: rename `surface` to `surfaceResource` and then do `auto surface =
SurfaceInterface::get(surfaceResource);`.
> inputmethod_interface.cpp:224
> +
> + auto ipsi = new InputPanelSurfaceInterface(nullptr);
> + ipsi->d->init(resource->client(), id, resource->version());
No abbreviations.
> inputmethod_interface.h:13
> +
> +#include "resource.h"
> +
Do we actually need it?
> inputmethod_interface.h:102
> +Q_SIGNALS:
> + void inputPanelSurfaceAdded(quint32 id, InputPanelSurfaceInterface
> *surface);
> +
API design nitpick: it would be nice to have a signal with only parameter -
InputPanelSurfaceInterface *surface. Is there a reason why
InputPanelSurfaceInterface can't have an id getter?
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D27338
To: apol, #kwin, #frameworks
Cc: zzag, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,
bruns
