D29231: Add keyboard_shortcuts_inhibit protocol
bport abandoned this revision. bport added a comment. Follow up on gitlab https://invent.kde.org/kde/kwayland-server/-/merge_requests/1 REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D29231 To: bport, zzag, davidedmundson, apol Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
ngraham added a dependent revision: D29272: Add support to keyboard shortcuts inhibitor. REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D29231 To: bport, zzag, davidedmundson, apol Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
davidedmundson added inline comments. INLINE COMMENTS > zzag wrote in keyboard_shortcuts_inhibit_interface.cpp:21 > 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. 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 REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D29231 To: bport, zzag, davidedmundson, apol Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
zzag added inline comments. INLINE COMMENTS > davidedmundson wrote in keyboard_shortcuts_inhibit_interface.cpp:21 > Q_DECL_PRIVATE 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. > keyboard_shortcuts_inhibit_interface.h:61 > + */ > +KeyboardShortcutsInhibitorInterface > *getShortcutsInhibitor(SurfaceInterface *surface, SeatInterface *seat) const; > + We usually don't prefix getters with "get". What about shortcutsInhibitor() or findInhibitior()? REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D29231 To: bport, zzag, davidedmundson, apol Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
davidedmundson added a comment.
Let's wait for the new kwaylandserver repo before pushing.
But ++, looks good.
INLINE COMMENTS
> keyboard_shortcuts_inhibit_interface.cpp:21
> +
> +class KeyboardShortcutsInhibitorInterface::Private : public
> QtWaylandServer::zwp_keyboard_shortcuts_inhibitor_v1
> +{
Q_DECL_PRIVATE
> keyboard_shortcuts_inhibit_interface.cpp:35
> +
> +KeyboardShortcutsInhibitorInterface::Private::Private(wl_resource *resource,
> int id, SurfaceInterface *surface,
> + SeatInterface *seat,
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.
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D29231
To: bport, zzag, davidedmundson, apol
Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh,
ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
crossi added a comment.
some code style related comments
INLINE COMMENTS
> test_keyboard_shortcuts_inhibitor_interface.cpp:109
> +m_manager = m_display.createKeyboardShortcutsInhibitManager(this);
> + QVERIFY(m_serverCompositor->isValid());
> +connect(m_serverCompositor, &CompositorInterface::surfaceCreated, this,
> [this](SurfaceInterface *surface) {
indentation
> test_keyboard_shortcuts_inhibitor_interface.cpp:154
> +for (int i = 0; i < 3; ++i) {
> +KWayland::Client::Surface *s
> =m_clientCompositor->createSurface(this);
> +m_clientSurfaces += s->operator wl_surface *();
add space after =
> display.h:328
> + */
> +KeyboardShortcutsInhibitManagerInterface
> *createKeyboardShortcutsInhibitManager(QObject *object);
> +
parameter should be `QObject *parent = nullptr`
> keyboard_shortcuts_inhibit_interface.h:69
> +friend class KeyboardShortcutsInhibitorInterface;
> +explicit KeyboardShortcutsInhibitManagerInterface(Display *d, QObject
> *parent);
> +void removeInhibitor(SurfaceInterface *const surface, SeatInterface
> *const seat);
parent parameter should be defaulted to nullptr
REPOSITORY
R127 KWayland
REVISION DETAIL
https://phabricator.kde.org/D29231
To: bport, zzag, davidedmundson, apol
Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh,
ngraham, bruns
D29231: Add keyboard_shortcuts_inhibit protocol
bport retitled this revision from "[WIP] Add keyboard_shortcuts_inhibit protocol" to "Add keyboard_shortcuts_inhibit protocol". REPOSITORY R127 KWayland REVISION DETAIL https://phabricator.kde.org/D29231 To: bport, zzag, davidedmundson, apol Cc: romangg, crossi, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
