WIP: FEAT(client): Add support to XDG Desktop Portal GlobalShortcuts #5976

aleixpol · 2022-11-25T18:57:35Z

This makes it possible to have global shortcuts on systems running the XDG Desktop Portal service. This is especially relevant on Wayland where we are not able to run a system-wide keylogger to get the global shortcuts triggers.

Fixes #5257

Checks

My commits follow the commit guidelines

WIP because I'm not very familiar because this is a codebase alien to me and I'm aware it's doing some nasty things. Still, I'd prefer to know how the maintainers want to do it rather than imagining myself what they want instead.

Note the GlobalShortcuts portal is merged to master but it still isn't released.

Krzmbrzl

Why is the implementation using the name "xdp" rather than "xdg"?

And I'm wondering: Given that this is such a new feature, do we want to keep the old wayland notice in-place and show, if on wayland and if the new impl is not available?

src/EnvUtils.h

Krzmbrzl · 2022-11-27T18:23:04Z

src/mumble/CMakeLists.txt

+	qt_add_dbus_interface(mumble_xdp_SRCS org.freedesktop.portal.GlobalShortcuts.xml globalshortcuts_portal_interface)
+	find_file(PORTALSREQUEST_XML share/dbus-1/interfaces/org.freedesktop.portal.Request.xml PATH_SUFFIXES share PATHS /usr ${CMAKE_INSTALL_PREFIX})
+	qt_add_dbus_interface(mumble_xdp_SRCS ${PORTALSREQUEST_XML} portalsrequest_interface)
+	target_sources(mumble_client_object_lib PRIVATE ${mumble_xdp_SRCS})


I feel like this should be outsourced into a dedicated cmake function implemented in a file inside the cmake directory. Then this place here could simply call that function.

Plus, it seems that we currently don't handle the case when the searched for file can't be found.

I moved both files to auxiliary_files.

I am not sure what you mean by adding the function. Do you still want it if we are not looking it up?

src/mumble/GlobalShortcut_xdp.cpp

src/mumble/GlobalShortcut_xdp.h

src/mumble/org.freedesktop.portal.GlobalShortcuts.xml

Krzmbrzl · 2022-11-27T18:44:16Z

And out of curiosity: Will this be a flatpak-specific thing or will this (likely) solve the issue for Wayland users in general (also for non-flatpak apps)?

aleixpol · 2022-11-28T00:56:49Z

And out of curiosity: Will this be a flatpak-specific thing or will this (likely) solve the issue for Wayland users in general (also for non-flatpak apps)?

Yes, this should solve it for every Wayland system. Also it can work on X11 provided it's properly implemented in the backend like we are doing in KDE/Plasma and I expect others will too.

Krzmbrzl

Sorry for the long delay since my last review - I have been rather busy

Krzmbrzl · 2022-12-28T11:29:24Z

src/mumble/GlobalShortcut.cpp

-	qlWaylandNote->setVisible(false);
-#ifdef Q_OS_LINUX
-	if (EnvUtils::waylandIsUsed()) {
-		// Our global shortcut system doesn't work properly with Wayland
-		qlWaylandNote->setVisible(true);
-	}
-#endif


If I understand things correctly, the new system only works, if DBus support is enabled. Therefore, I think we probably want to keep showing this note in cases in which Mumble is built without DBus.
(In case Mumble is built with DBus, the USE_DBUS macro will be defined)

Or maybe even better: show the note, if !GlobalShortcutXdp::isAvailable()

Any linux installation without dbus will be broken in many ways. Are you sure you want to keep the case around?

This is not about the distro having no DBus support. But (for whatever reason) we have an option to build Mumble without DBus support.
I'll check with the rest of the team whether we can simply remove that option though. If we do, then there is no need for you to make these kinds of changes... I'll keep you posted

Alright, we decided to drop the dbus option. Thus, you can simply assume to always have DBus available 👍

src/mumble/GlobalShortcut_unix.cpp

src/mumble/GlobalShortcut_xdp.cpp

src/mumble/GlobalShortcut_xdp.h

Krzmbrzl · 2022-12-28T11:38:15Z

src/mumble/Settings.cpp

-#ifdef Q_OS_LINUX
-	if (EnvUtils::waylandIsUsed()) {
-		// Due to the issues we're currently having on Wayland, we disable shortcuts by default
-		bShortcutEnable = false;
-	}
-#endif


This should probably get the same treatment as the note in the settings UI

src/mumble/GlobalShortcut_xdp.cpp

Krzmbrzl · 2022-12-28T11:47:34Z

src/mumble/GlobalShortcut_xdp.cpp

+        // TODO
+        return {};


Just marking this so we don't lose track of this TODO

So to address this TODO we'll need to use an interface like this:
https://invent.kde.org/libraries/xdg-portal-test-kde/-/merge_requests/18

This means depending on Qt::GuiPrivate and libwaylandclient. If you are interested in this I can include it in this PR.

Hm. This seems rather hacky. What are the implications of simply leaving the implementation as it is? Aka: what exactly is the parent window ID being used for?
Will this perhaps only become relevant when dealing with multiple Mumble instances?

The parent window is simply to inform the portal who is calling this and which window the dialog should be on top.

The dialog will appear on top and focussed anyway, so it shouldn't be a big deal. FWIW, there will be public API for this in Qt 6 (6.5, if I'm not mistaken). If you don't want to depend on private API, just delaying until Qt 6 is a good option.

Okay in that case it indeed seems a good idea to simply delay this 👍

This makes it possible to have global shortcuts on systems running the XDG Desktop Portal service. This is especially relevant on Wayland where we are not able to run a system-wide keylogger to get the global shortcuts triggers. Fixes mumble-voip#5257

CounterPillow · 2023-02-16T19:17:23Z

Two awkward things I've noticed which may or may not be related to the implementation in this PR:

you can't bind mouse buttons to global shortcuts. I assume this is either a deficiency specifically with this PR or with KDE's GlobalShortcuts interface ignoring all mouse presses
pressing a modifier while pressing a key does not allow for that key to be picked up by the shortcut, so if I bind F13 (or as KDE calls it, "Tools") to push-to-talk, I also need to bind shift+Tools, shift+ctrl+Tools, ctrl+Tools, alt+tools, ... which may be a deficiency in the entire protocol.

aleixpol · 2023-02-18T02:19:45Z

you can't bind mouse buttons to global shortcuts.

Correct, we don't have global shortcuts triggered my mouse in Plasma. It should be supported by Plasma or the Desktop Environment of choice of the user anyway.

pressing a modifier while pressing a key does not allow for that key to be picked up by the shortcut,

Again, this is Plasma deciding that they are not the same shortcut and not triggering it. There's not much we can (or should) do from this side. I suggest we continue this discussion in the bug report you created: https://bugs.kde.org/show_bug.cgi?id=465867

I suggest explaining a bit more on the bug report why it's important for anymodifier to trigger the subject's shortcut. If we have clear use cases it will be easier to get it acted on.

SopaDeMacaco-UmaDelicia · 2023-05-28T01:47:35Z

What's the status of this PR? Can't wait to enjoy global shortcuts in Mumble using KDE Wayland.

aleixpol force-pushed the work/xdp-support branch from e9f84da to c1fd93f Compare November 25, 2022 19:20

aleixpol mentioned this pull request Nov 27, 2022

global-shortcuts: Signal arguments are outputs flatpak/xdg-desktop-portal#917

Merged

Krzmbrzl added client linux feature-request This issue or PR deals with a new feature GlobalShortcuts labels Nov 27, 2022

Krzmbrzl requested changes Nov 27, 2022

View reviewed changes

Krzmbrzl marked this pull request as draft November 27, 2022 19:06

aleixpol force-pushed the work/xdp-support branch from e00c411 to a59e8f3 Compare November 28, 2022 00:54

aleixpol mentioned this pull request Dec 15, 2022

Global keyboard shortcut portal flatpak/xdg-desktop-portal#624

Closed

Krzmbrzl reviewed Dec 28, 2022

View reviewed changes

aleixpol force-pushed the work/xdp-support branch 2 times, most recently from 6fbe414 to 62bb6d9 Compare January 2, 2023 15:10

aleixpol force-pushed the work/xdp-support branch from 62bb6d9 to 0e0f075 Compare January 2, 2023 17:17

Aviana mentioned this pull request Apr 4, 2023

[WIP] Global Shortcuts impl hyprwm/xdg-desktop-portal-hyprland#35

Merged

5 tasks

Hartmnt mentioned this pull request Jun 13, 2023

Enable push to talk shortcut button settings in wayland session #6146

Closed

Aviana mentioned this pull request Dec 7, 2023

Global Shortcuts Portal silent propagation of shortcuts flatpak/xdg-desktop-portal#1101

Open

This was referenced May 25, 2024

Lets get org.freedesktop.portal.GlobalShortcuts off the ground Skycoder42/QHotkey#99

Closed

xdg-desktop-portal org.freedesktop.portal.GlobalShortcuts Skycoder42/QHotkey#89

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: FEAT(client): Add support to XDG Desktop Portal GlobalShortcuts #5976

WIP: FEAT(client): Add support to XDG Desktop Portal GlobalShortcuts #5976

aleixpol commented Nov 25, 2022

Krzmbrzl left a comment

Krzmbrzl Nov 27, 2022

aleixpol Nov 28, 2022

Krzmbrzl commented Nov 27, 2022

aleixpol commented Nov 28, 2022

Krzmbrzl left a comment

Krzmbrzl Dec 28, 2022

aleixpol Jan 2, 2023

Krzmbrzl Jan 2, 2023

Krzmbrzl Jan 2, 2023

Krzmbrzl Dec 28, 2022

Krzmbrzl Dec 28, 2022

aleixpol Jan 2, 2023

Krzmbrzl Jan 2, 2023

aleixpol Jan 2, 2023

Krzmbrzl Jan 4, 2023

CounterPillow commented Feb 16, 2023

aleixpol commented Feb 18, 2023

SopaDeMacaco-UmaDelicia commented May 28, 2023

WIP: FEAT(client): Add support to XDG Desktop Portal GlobalShortcuts #5976

Are you sure you want to change the base?

WIP: FEAT(client): Add support to XDG Desktop Portal GlobalShortcuts #5976

Conversation

aleixpol commented Nov 25, 2022

Checks

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krzmbrzl commented Nov 27, 2022

aleixpol commented Nov 28, 2022

Krzmbrzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CounterPillow commented Feb 16, 2023

aleixpol commented Feb 18, 2023

SopaDeMacaco-UmaDelicia commented May 28, 2023