Add keyboard_shortcuts_inhibit protocol
AbandonedPublic

Authored by bport on Apr 27 2020, 3:24 PM.

Details

Diff Detail

Repository
R127 KWayland
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 26006
Build 26024: arc lint + arc unit
bport created this revision.Apr 27 2020, 3:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 27 2020, 3:24 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bport requested review of this revision.Apr 27 2020, 3:24 PM
crossi added a subscriber: crossi.Apr 27 2020, 3:37 PM
crossi added inline comments.
src/server/keyboard_shortcuts_inhibit_interface.cpp
93

indentation is missing

src/server/keyboard_shortcuts_inhibit_interface.h
40

Should be const

bport updated this revision to Diff 81355.Apr 27 2020, 3:40 PM

Cyril feedback

bport edited the summary of this revision. (Show Details)Apr 27 2020, 3:40 PM
bport updated this revision to Diff 81358.Apr 27 2020, 3:43 PM
bport marked an inline comment as done.

added const to wrong line

apol added a comment.Apr 27 2020, 3:46 PM

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

You'll want to connect it to the respective client here, it's not a global anymore.

bport updated this revision to Diff 81361.Apr 27 2020, 4:07 PM

Use client not display for KeyboardShortcutsInhibitorInterface

zzag requested changes to this revision.Apr 27 2020, 4:41 PM

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

What if the client requested version < s_version? You need to create the resource manually.

130

We probably need to ask folks in #wayland whether inhibitors outlive the manager object.

src/server/keyboard_shortcuts_inhibit_interface.h
33

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

We probably don't need it to be public. Only emit inhibitorCreated.

This revision now requires changes to proceed.Apr 27 2020, 4:41 PM
romangg added inline comments.
src/server/keyboard_shortcuts_inhibit_interface.cpp
130

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.

bport updated this revision to Diff 81385.Apr 27 2020, 9:54 PM

fix test, and take in consideration zzag feedback

bport updated this revision to Diff 81386.Apr 27 2020, 9:56 PM

fix qobject macro indentation

apol added inline comments.Apr 27 2020, 11:49 PM
src/server/keyboard_shortcuts_inhibit_interface.cpp
145

Just return m_inhibitors.value({ surface, seat }) to save a look-up and a few lines of code.

zzag added a comment.Apr 28 2020, 5:47 AM

Thanks, this patch looks good to me. Although it would be nice to see the kwin side before leaving a +1. :)

bport updated this revision to Diff 81491.Apr 29 2020, 8:43 AM

Simplify code arround QHash and QPair

bport retitled this revision from [WIP] Add keyboard_shortcuts_inhibit protocol to Add keyboard_shortcuts_inhibit protocol.Apr 29 2020, 9:24 AM

some code style related comments

autotests/server/test_keyboard_shortcuts_inhibitor_interface.cpp
110

indentation

155

add space after =

src/server/display.h
328

parameter should be QObject *parent = nullptr

src/server/keyboard_shortcuts_inhibit_interface.h
70

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

Q_DECL_PRIVATE

36

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.

zzag added inline comments.Apr 29 2020, 10:10 AM
src/server/keyboard_shortcuts_inhibit_interface.cpp
22

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

We usually don't prefix getters with "get". What about shortcutsInhibitor() or findInhibitior()?

src/server/keyboard_shortcuts_inhibit_interface.cpp
22

yeah, that one

there are valid arguments against nested private classes so it would be really nice if we revisit this topic in the new repo.

Sure