Details
- Reviewers
zzag davidedmundson apol
Diff Detail
- Repository
- R127 KWayland
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 25988 Build 26006: arc lint + arc unit
Code looks good overall, I'd say you'll get to polish the weird bits when developing the kwin side.
In fact, this probably could be implemented within kwin considering what we discussed last week. We could remove the weirdness we have to keep its ABI.
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 26 ↗ | (On Diff #81351) | You'll want to connect it to the respective client here, it's not a global anymore. |
I don't want to be selfish, but I'm not really used to the coding style in this patch. Could you please move method definitions outside class declarations?
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 26 ↗ | (On Diff #81351) | What if the client requested version < s_version? You need to create the resource manually. |
| 130 ↗ | (On Diff #81351) | We probably need to ask folks in #wayland whether inhibitors outlive the manager object. |
| src/server/keyboard_shortcuts_inhibit_interface.h | ||
| 33 ↗ | (On Diff #81351) | Indentation. Also, it's probably a minor thing, but it's really nice when Q_OBJECT is separated from code below by a single empty line (cosmetics). |
| 61 ↗ | (On Diff #81351) | We probably don't need it to be public. Only emit inhibitorCreated. |
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 130 ↗ | (On Diff #81351) | In general objects created through some interface can outlive the interface bind if not otherwise specified in the protocol definition. I.e. the todo can be removed. |
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 145 ↗ | (On Diff #81351) | Just return m_inhibitors.value({ surface, seat }) to save a look-up and a few lines of code. |
Thanks, this patch looks good to me. Although it would be nice to see the kwin side before leaving a +1. :)
some code style related comments
| autotests/server/test_keyboard_shortcuts_inhibitor_interface.cpp | ||
|---|---|---|
| 110 ↗ | (On Diff #81351) | indentation |
| 155 ↗ | (On Diff #81351) | add space after = |
| src/server/display.h | ||
| 328 ↗ | (On Diff #81351) | parameter should be QObject *parent = nullptr |
| src/server/keyboard_shortcuts_inhibit_interface.h | ||
| 70 ↗ | (On Diff #81351) | parent parameter should be defaulted to nullptr |
Let's wait for the new kwaylandserver repo before pushing.
But ++, looks good.
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 22 ↗ | (On Diff #81351) | Q_DECL_PRIVATE |
| 36 ↗ | (On Diff #81351) | It's not clear what resource this argument is referring to. I think from reading the code it's the resource of the manager? At which point "parentResource" would be a better name. Or separate client, id, version args. |
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 22 ↗ | (On Diff #81351) | You've probably meant Q_DECL_HIDDEN, right? On an unrelated note: there are valid arguments against nested private classes so it would be really nice if we revisit this topic in the new repo. |
| src/server/keyboard_shortcuts_inhibit_interface.h | ||
| 62 ↗ | (On Diff #81351) | We usually don't prefix getters with "get". What about shortcutsInhibitor() or findInhibitior()? |
| src/server/keyboard_shortcuts_inhibit_interface.cpp | ||
|---|---|---|
| 22 ↗ | (On Diff #81351) | yeah, that one
Sure |