D27338: Provide an initial implementation for input-method-unstable-v1

[Vlad Zahorodnii]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200325/1ee7c409/attachment-0001.html>