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

Reply via email to