Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a global shortcut portal #711

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

aleixpol
Copy link
Contributor

It's designed so that applications can register actions that can be
triggered globally (i.e. regarless of the system's state, like focus).

WIP because:

  • It's untested
  • There's no implementation for it
  • Needs reviews

Fixes #624

Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, personally I could really use a few examples here to better understand what the intention is of this portal. I get that it would address the simple "ctrl+c" shortcut type but it seems overspecced for that simple case. And I think it may be underspecced for the more complex cases that require key repeats. So I'm wondering: what are the specific use-cases you are trying to solve here?

data/org.freedesktop.impl.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
src/globalshortcuts.h Outdated Show resolved Hide resolved
@aleixpol aleixpol force-pushed the work/global-shortcut branch from 472b2dd to b83f56e Compare March 8, 2022 17:00
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Mar 9, 2022

I get that it would address the simple "ctrl+c" shortcut type but it seems overspecced for that simple case

My understanding is that this is for global shortcuts that are always active, even while an unrelated application has keyboard focus. Instead of "copy" or "zoom in", think more like "start recording".

While an application has focus, it can receive Ctrl+C (or similar) in the usual X11 or Wayland way, without needing portals.

@aleixpol aleixpol force-pushed the work/global-shortcut branch from b83f56e to 7986af5 Compare March 9, 2022 15:48
@jadahl
Copy link
Collaborator

jadahl commented Mar 9, 2022

Lets say we have an application that can control the sun shade of a physical window, and that it wants to bind the actions "roll up sun shade" and "roll down sun shade" using the global shortcut portal to some two key bindings.

Could you describe step by step how the application does this, when a portal dialog will be presented, and how the application does the second time it launches?

@aleixpol aleixpol force-pushed the work/global-shortcut branch from 7986af5 to 95e7724 Compare March 9, 2022 15:57
@aleixpol
Copy link
Contributor Author

aleixpol commented Mar 9, 2022

Could you describe step by step how the application does this, when a portal dialog will be presented, and how the application does the second time it launches?

app:

void activatedCallback(name)
{
    if (name == "shade-up")
        shade->up();
    else if (name == "shade-down")
        shade->down();
}

request = CreateSession({
    "shade-up", i18n("Rolls the sun shades up"),
    "shade-down", i18n("Rolls the sun shades down")
})

request.exec(); // Wait for it to finish...
session = request.result()["session_handle"];

connect(session.activated, activatedCallback);

if (we really want to make sure the global shortcuts are active) {
    request = ListShortcuts(session);
    request.exec(); // Wait for it to finish...
    shortcuts = request.result(); // [2]
    
    if (shortcuts["shade-up"] == null) {
        request = Bind("shade-up"); // The portal might prompt here [1]
        request.exec(); // Wait for it to finish...
        request.makeSureItDidNotFail();
    }
    if (shortcuts["shade-down"] == null) {
        request = Bind("shade-down"); // The portal might prompt here [1]
        request.exec(); // Wait for it to finish...
        request.makeSureItDidNotFail();
    }
}

// We update the UI for the user to know how to trigger it
{
    request = ListShortcuts(session); // [3]
    request.exec(); // Wait for it to finish...
    shortcuts = request.result();
    
    ui.shadeUpLabel.text = shortcuts["shade-up"]
    ui.shadeDownLabel.text = shortcuts["shade-down"]
}
  1. The portal will always be notified and asked to bind so the application can call this again if the binding needs changing.
  2. on the second executions the portal should restore the bindings that were initially registered. We could consider including the ListShortcuts output to the CreateSession's request output
  3. We call ListShortcuts twice for readability, Bind() does offer this information in the request's output

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few comments about the design of the API. None of the C code was reviews, I'll review the implementation once we figure out the D-Bus bits.

Something I'd like us to think through is the security model of this API. The current iteration implies that applications would need to request the same shortcuts on startup, and no permissions are stored anywhere. This can be an annoying behavior. On the other hand, allowing any app to request global shortcuts without any extra check is not ideal either. So the question is: how would we implement apps only requesting permissions once?

data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
@jadahl
Copy link
Collaborator

jadahl commented Mar 16, 2022

I agree with @GeorgesStavracas that it'd be good to be able to batch multiple Bind requests into a single BindShortcuts as described above.

Thinking more about the permission model would indeed be good. What needs to be ensured is that: applications should not be allowed to arbitrarily bind shortcuts without user consent; at least the API must make sure that a backend can implement such semantics. It'd also be good to allow an application to be able to request new bindings, or change existing ones, thus the initial set of bindings being set up at session construction is a bit awkward.

Given this, maybe we can take inspiration from the screen cast session restore functionality, by making the following changes:

  1. Replace the shortcuts parameter in CreateSession with a restore_token string,
  2. Add a restore_token to the data passed via the response signal emitted after CreateSession
  3. Replace Bind with BindShortcuts to allow for binding in batches (thus setting up multiple bindings in the same dialog)
  4. Pass internationalized description that was previously part of the shortcuts parameter in CreateSession together with BindShortcuts

With this, the flow would be altered slightly, but I believe it should be possible to implement the same user interface as you described in the issue:

void activatedCallback(name)
{
    if (name == "shade-up")
        shade->up();
    else if (name == "shade-down")
        shade->down();
}

options = {};
if (has_restore_token)
    options["restore_token"] = get_saved_restore_token();

request = CreateSession(options);

response = request.exec(); // Wait for it to finish...
session = request.result()["session_handle"];

connect(session.activated, activatedCallback);

response_restore_token = response["restore_token"];

app_config_store.store_shortcut_session_token(response_restore_token)

needs_to_bind = False;
existing_bindings = []
if (response_restore_token is null)
    needs_to_bind = True;
else {
    existing_bindings = ListShortcuts(session);
    if (existing_bindings.length < wanted_bindings.length and should_nag_user_about_bindings())
        needs_to_bind = True
}

if (needs_to_bind) {
    new_bindings = wanted_bindings.remove(existing_bindings)
    BindShortcuts(new_bindings); // <--- will show dialog
}

Edit: github decided to treat some key binding as "save" before the comment was ready, should be fixed now.

@GeorgesStavracas
Copy link
Member

Another question that just popped up: should this protocol support binding the same shortcut to multiple key combinations?

@jadahl
Copy link
Collaborator

jadahl commented Mar 17, 2022

Another question that just popped up: should this protocol support binding the same shortcut to multiple key combinations?

Sounds like something an impl backend can do without involving any APIs.

@swick
Copy link
Contributor

swick commented Mar 17, 2022

Sounds like something an impl backend can do without involving any APIs.

If we want to give apps access to a string which describes what the shortcut is bound to this might not be so straightforward.

@GeorgesStavracas GeorgesStavracas marked this pull request as draft March 19, 2022 12:21
@GeorgesStavracas
Copy link
Member

Converted to a Draft so no one (read: me) accidentally merges it

@aleixpol aleixpol force-pushed the work/global-shortcut branch from 95e7724 to e43ee47 Compare March 21, 2022 13:41
@aleixpol
Copy link
Contributor Author

Thinking more about the permission model would indeed be good. What needs to be ensured is that: applications should not be allowed to arbitrarily bind shortcuts without user consent; at least the API must make sure that a backend can implement such semantics.

There's no way for an app to register here. We have not even specified how that would work. I'm afraid we might be miscommunicating.

It'd also be good to allow an application to be able to request new bindings, or change existing ones, thus the initial set of bindings being set up at session construction is a bit awkward.

It could be nice to show some UI to let the portal handle the UX for all global shortcuts. I wouldn't expect this to be how every app does it though, as I expect apps to want to show their own UI for listing these shortcuts.

Given this, maybe we can take inspiration from the screen cast session restore functionality, by making the following changes:

1. Replace the `shortcuts` parameter in `CreateSession` with a `restore_token` string,
2. Add a `restore_token` to the data passed via the response signal emitted after `CreateSession`
3. Replace `Bind` with `BindShortcuts` to allow for binding in batches (thus setting up multiple bindings in the same dialog)
4. Pass internationalized description that was previously part of the `shortcuts` parameter in `CreateSession` together with `BindShortcuts`

My understanding is that we should keep the shortcuts an app has requested by its name, no need to keep a token. I don't know in which cases we'd consider the application a different one.

In practice, we can consider shortcuts to be static in a specific version of an app and could change between versions but it should only change slightly.

I do want us to be able to offer a UX that doesn't necessarily have to show UI from the portals, as we've seen this lead us to rather awkward workflows with cross-platform apps.

@aleixpol aleixpol force-pushed the work/global-shortcut branch from e43ee47 to 1c53a39 Compare March 21, 2022 14:09
@aleixpol
Copy link
Contributor Author

aleixpol commented Mar 21, 2022

Another question that just popped up: should this protocol support binding the same shortcut to multiple key combinations?

You can also give from gnome's portal a description trigger that is Ctrl+A or Meta+Z, this would allow us to make the supported feature set be less tied to the spec.

Edit: description -> trigger

@GeorgesStavracas
Copy link
Member

I feel like we're set on a path of confusion with the terminology here. I'd like to propose the following:

  • id: unique identifier of a shortcut within the application
  • description: user-visible description of the shortcut, ideally explaining what it will do when triggered
  • trigger: the key combination that triggers the given shortcut

Objections?

@swick
Copy link
Contributor

swick commented Mar 21, 2022

I wouldn't expect this to be how every app does it though, as I expect apps to want to show their own UI for listing these shortcuts.

I still don't get how this is supposed to work.

For example if you want to delete a shortcut and the app doesn't delete it but deletes it in its UI you suddenly have a shortcut bound to a trigger which you are not aware of. How does it work if you want to bind multiple triggers? How does it work if you bind triggers which require rich UI? How do you handle conflicts and potential conflicts?

All of that is pretty straightforward if you do all of it in the application (X11 style) or all of it in the system UI (portal backend) but you want some kind of hybrid which makes everything much harder.

@aleixpol aleixpol force-pushed the work/global-shortcut branch from 1c53a39 to 0b5f041 Compare March 23, 2022 17:41
Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate Apol is still working on the previous round of review, but I had a few comments to flush

@aleixpol
Copy link
Contributor Author

aleixpol commented Mar 25, 2022

@aleixpol aleixpol force-pushed the work/global-shortcut branch 3 times, most recently from 5693f05 to c0318fd Compare March 26, 2022 01:18
data/org.freedesktop.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
data/org.freedesktop.impl.portal.GlobalShortcuts.xml Outdated Show resolved Hide resolved
@handle: Object path for the #org.freedesktop.impl.portal.Request object representing this call
@session_handle: Object path for the #org.freedesktop.impl.portal.Session object representing the session
@parent_window: Identifier for the application window, see <link linkend="parent_window">Common Conventions</link>
@shortcuts: The identifier of the shortcuts we intend to register, empty for all shortcuts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if one tries to bind shortcuts not part of the CreateSession call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should error out. I am not sure how this is generally communicated, do you know what would be a good example?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not allowed to bind shortcuts that wasn't specified in CreateSession the documentation should specify that it'll fail. I guess it will via the response signal.

How would an application that doesn't know its wanted global shortcuts immediately add new ones? Does it have to have one session per added binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can close the session and start a new one with the previous + the added one. This is how I imagined it at least.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Re-creating the session is racy; if one triggers a binding right at the time the app decides to recreate the session, it can be lost. Maybe not a big deal since it's very unlikely to happen very often. Then again, could just as well be one session per binding couldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-creating the session is racy; if one triggers a binding right at the time the app decides to recreate the session, it can be lost. Maybe not a big deal since it's very unlikely to happen very often.

Correct. It shouldn't happen though during general runtime of the app.

Then again, could just as well be one session per binding couldn't it?

Yes.

@GeorgesStavracas
Copy link
Member

@aleixpol i think this PR now needs to update the Meson files too. Shouldn't be hard to add, and won't impact review too much, but I'd appreciate if you could add it too since next xdg-desktop-portal release will be done with Meson 🙂

@aleixpol aleixpol force-pushed the work/global-shortcut branch from a656378 to d37bb4f Compare September 5, 2022 15:32
@aleixpol
Copy link
Contributor Author

aleixpol commented Sep 5, 2022

@aleixpol i think this PR now needs to update the Meson files too.

That explains the CI exploding, should be addressed

@orowith2os orowith2os mentioned this pull request Sep 8, 2022
It's designed so that applications can register actions that can be
triggered globally (i.e. regarless of the system's state, like focus).
@aleixpol aleixpol force-pushed the work/global-shortcut branch from d37bb4f to 283e0d2 Compare September 21, 2022 14:42
Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There still are a few things to correct here, but I think it'll be more productive to do that myself 🙂

@GeorgesStavracas GeorgesStavracas merged commit bed5fd6 into flatpak:main Sep 22, 2022
@aleixpol aleixpol deleted the work/global-shortcut branch September 22, 2022 14:38
@aleixpol
Copy link
Contributor Author

Glad to see it's in, I'd have expected a PR into this branch but I guess that will work anyway. Thanks, please poke me for the polishes.

@aleixpol
Copy link
Contributor Author

I apologise for being a bit obtuse. Will a shortcut exclusivity option be addressed in x-d-p, or will this be left up to the backend implementations? With shortcut exclusivity I refer to sending the keys pressed to the focused application as well as the input event for the activated shortcut.

This protocol is only for system-wide shortcuts. As a foscussed application you can always do as you wish with the input you are given.

If there's any public IRC channel to catch up to internal communications that I am unaware of please let me know.

No, all the communication to put this together in this Pull Request.

@RichardFevrier
Copy link

Would it be possible with this portal for a third party app (KDE Settings/GNOME Settings/other...) to be able to list and manage all global shortcuts from every app using them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global keyboard shortcut portal