Provide an initial implementation for input-method-unstable-v1
Needs ReviewPublic

Authored by apol on Feb 12 2020, 12:51 AM.

Details

Reviewers
None
Group Reviewers
KWin
Frameworks
Summary

Makes it possible to implement the protocol in your favourite
compositor.

Test Plan

Used it with maliit and weston-keyboard

Diff Detail

Repository
R127 KWayland
Branch
arcpatch-D27338
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24273
Build 24291: arc lint + arc unit
apol created this revision.Feb 12 2020, 12:51 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 12 2020, 12:51 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Feb 12 2020, 12:51 AM
zzag added a subscriber: zzag.Wed, Mar 25, 8:18 AM
zzag added inline comments.
src/server/display.cpp
49

Stray new line. Please remove it.

src/server/inputmethod_interface.cpp
26

Add Q_DECL_HIDDEN.

126

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);
146–147

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.

180

Shouldn't InputPanelSurfaceInterface be also a subclass of SurfaceRole?

200–201

Naming nitpick: in FooPrivate classes, we avoid putting m_.

221

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);.

225

No abbreviations.

src/server/inputmethod_interface.h
14

Do we actually need it?

103

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?

apol marked 6 inline comments as done.Thu, Mar 26, 2:57 AM
apol added inline comments.
src/server/inputmethod_interface.cpp
26

If it makes you happy. But we're building with opt-in export symbols, it shouldn't make much of a difference.

180

I don't know, just looked at it and it doesn't seem that useful?

200–201

I would rather not, otherwise when implementing private members they read like local variables.

apol updated this revision to Diff 78514.Thu, Mar 26, 2:57 AM
apol marked an inline comment as done.

Address comments

apol updated this revision to Diff 78515.Thu, Mar 26, 2:58 AM

rebase

zzag added inline comments.Thu, Mar 26, 7:51 AM
src/server/inputmethod_interface.cpp
129–130

Destroy q in zwp_input_method_context_v1_destroy_resource(). In the destructor request, we usually want to destroy only the resource.

180

zwp_input_panel_surface_v1 defines a surface role (based on weston code) so it should be a subclass of SurfaceRole. In long term, we want to do something like

SurfaceRole *role = SurfaceRole::get(surface);
if (role) {
    wl_resource_post_error(resource->handle, ZMY_SHELL_SURFACE_ERROR_ROLE,
                           "Cannot reassign surface role"); // DIE!
    return;
}
182

We need to destroy the wl_resource for zwp_input_panel_surface_v1 when the associated surface is destroyed.

200–201

Frameworks' policies have no a single word about this so I guess it's okay(?) to put m_. I asked to drop m_ because Qt folks seem to prefer not to put m_ in private classes.

I would appreciate if you bring this topic up to the discussion in #kde-frameworks-devel since we're not consistent with naming in private classes.

apol marked 4 inline comments as done.Thu, Mar 26, 2:45 PM
apol updated this revision to Diff 78549.Thu, Mar 26, 2:45 PM

address comments

apol updated this revision to Diff 78553.Thu, Mar 26, 2:54 PM

forgot to save the file

zzag added inline comments.Thu, Mar 26, 3:01 PM
src/server/inputmethod_interface.cpp
129

Don't call wl_resource_destroy() in foobar_destroy_resource(), it's dangerous.

void zwp_input_method_context_v1_destroy(Resource *resource)
{
    wl_resource_destroy(resource->handle);
}

void zwp_input_method_context_v1_destroy_resource(Resource *resource)
{
    Q_UNUSED(resource)
    q->deleteLater();
}
apol updated this revision to Diff 78557.Thu, Mar 26, 3:08 PM

vlad++

zzag added inline comments.Thu, Mar 26, 3:28 PM
src/server/inputmethod_interface.cpp
153–154

I don't understand why we need resourceMap() here. zwp_input_method_context_v1 is not a global so I would expect to see something like

void InputMethodContextInterface::sendCommitState(uint32 serial)
{
    d->send_commit_state();
}
206–210

Only delete q.

zzag added a comment.Mon, Mar 30, 2:13 PM

If it makes you happy. But we're building with opt-in export symbols, it shouldn't make much of a difference.

After reading some inline comments in D28295, I think it would be better to get rid of the nested private class thing. We won't need to use Q_DECL_HIDDEN and this is what Qt folks do. :-)

After reading some inline comments in D28295, I think it would be better to get rid of the nested private class thing. We won't need to use Q_DECL_HIDDEN and this is what Qt folks do. :-)

We can't just make up policy changes ad-hoc on a review request to make it different to every other class in KWayland.

zzag added a comment.Mon, Mar 30, 3:53 PM

We can't just make up policy changes ad-hoc on a review request to make it different to every other class in KWayland.

Heh, if we really don't want to deviate from the rest of KWayland, then we have to use kwaylandscanner. Not having nested private classes would be the least thing that makes InputMethodInterface, InputMethodContextInterface, InputPanelInterface, and InputPanelSurfaceInterface different from any other class in KWayland; but I see your point. Anyway, we can live with nested private classes as long as people don't forget to mark them Q_DECL_HIDDEN. Should we maybe also start a discussion in kde-frameworks-devel on this matter?

apol added a comment.Mon, Mar 30, 5:16 PM

Can we not have the discussion here? I added them already.