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

GlobalShortcuts: Add a request to configure shortcuts #1661

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sodivad
Copy link
Contributor

@Sodivad Sodivad commented Mar 11, 2025

As mentioned on Matrix this allows an application to steer their users to the place where they can configure global shortcutcs of the application.

kde portal impl: https://invent.kde.org/plasma/xdg-desktop-portal-kde/-/commit/a8b7532e33d98bdaf034a92c624bc63adaceec5c
client side impl in ashpd: Sodivad/ashpd@21be0f5

@Sodivad Sodivad force-pushed the configureshortcuts branch from 145c11a to d32ae8f Compare March 11, 2025 16:08
@Sodivad Sodivad force-pushed the configureshortcuts branch 4 times, most recently from 8a6f3b1 to 3fa1731 Compare March 12, 2025 15:41
@swick
Copy link
Collaborator

swick commented Mar 13, 2025

Seems like a useful addition. Just a few details to iron out, but I'd also like to have an implementation before we merge this.

As mentioned on Matrix this allows an application to steer
their users to the place where they can configure global shortcutcs
of the application.

Signed-off-by: David Redondo <[email protected]>
@Sodivad Sodivad force-pushed the configureshortcuts branch from 3fa1731 to 0b255f4 Compare March 19, 2025 15:08
@Sodivad
Copy link
Contributor Author

Sodivad commented Mar 19, 2025

Seems like a useful addition. Just a few details to iron out, but I'd also like to have an implementation before we merge this.

implementation of what? I did a backend impl, see the description

@swick
Copy link
Collaborator

swick commented Mar 21, 2025

I missed that this already has a backend impl.

g_variant_builder_end (&options_builder),
NULL,
shortcuts_configure_cb,
invocation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have to keep the invocation alive, so g_object_ref here, and assign to an autoptr (or otherwise unref at the end) in shortcuts_configure_cb.

Copy link
Collaborator

@swick swick left a comment

Choose a reason for hiding this comment

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

With the issues resolved this all LGTM.

{
g_dbus_error_strip_remote_error (error);
g_warning ("Failed to configure shortcuts: %s", error->message);
g_dbus_method_invocation_return_gerror(invocation, error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
g_dbus_method_invocation_return_gerror(invocation, error);
g_dbus_method_invocation_return_gerror (invocation, error);

return;
}

xdp_dbus_global_shortcuts_complete_configure_shortcuts(XDP_DBUS_GLOBAL_SHORTCUTS(global_shortcuts), invocation);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
xdp_dbus_global_shortcuts_complete_configure_shortcuts(XDP_DBUS_GLOBAL_SHORTCUTS(global_shortcuts), invocation);
xdp_dbus_global_shortcuts_complete_configure_shortcuts (XDP_DBUS_GLOBAL_SHORTCUTS (global_shortcuts), invocation);

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.

5 participants