Makes it possible to implement the protocol in your favourite
compositor.
Details
- Reviewers
- None
- Group Reviewers
KWin Frameworks
Used it with maliit and weston-keyboard
Diff Detail
- Repository
- R127 KWayland
- Branch
- arcpatch-D27338
- Lint
Lint Errors Excuse: xxx Severity Location Code Message Error src/server/inputmethod_interface.h:123 Cppcheck unknownMacro Error src/server/inputmethod_interface.h:123 Cppcheck unknownMacro Error src/server/tablet_interface.h:80 Cppcheck unknownMacro - Unit
No Unit Test Coverage - Build Status
Buildable 24188 Build 24206: arc lint + arc unit
src/server/display.cpp | ||
---|---|---|
49 | Stray new line. Please remove it. | |
src/server/inputmethod_interface.cpp | ||
25 | Add Q_DECL_HIDDEN. | |
125 | 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); | |
145–146 | 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. | |
179 | Shouldn't InputPanelSurfaceInterface be also a subclass of SurfaceRole? | |
199–200 | Naming nitpick: in FooPrivate classes, we avoid putting m_. | |
220 | 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);. | |
224 | No abbreviations. | |
src/server/inputmethod_interface.h | ||
13 | Do we actually need it? | |
102 | 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? |
src/server/inputmethod_interface.cpp | ||
---|---|---|
25 | If it makes you happy. But we're building with opt-in export symbols, it shouldn't make much of a difference. | |
179 | I don't know, just looked at it and it doesn't seem that useful? | |
199–200 | I would rather not, otherwise when implementing private members they read like local variables. |
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. | |
179 | 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. | |
199–200 | 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. |
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(); } |
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.
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?