-
Notifications
You must be signed in to change notification settings - Fork 42
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 globalshortcuts demo #214
base: master
Are you sure you want to change the base?
Conversation
I have a video, but github doesn't seem to accept it. It's here instead: https://gitlab.gnome.org/GNOME/xdg-desktop-portal-gnome/-/issues/47#note_2144834 |
<child> | ||
<object class="AdwPreferencesGroup"> | ||
<property name="title" translatable="yes">Global Shortcuts</property> | ||
<property name="description" translatable="yes">Comma-separated list of shortcuts to request, in the form: "name:description:optional trigger"</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of putting this in a single entry, instead I would do something like notifications buttons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I do that? Is there an example I could use as a guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are examples in both emails attachments and notifications buttons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and I failed. Those examples don't show how to modify a property of a widget inside the list. Finding the inside widgets in the obvious way (by treating the widget as a list of widgets) fails in incomprehensible ways.
} | ||
|
||
impl GlobalShortcutsPage { | ||
async fn start_session(&self) -> ashpd::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry but this function is too huge, please split it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what hugeness standard we're aiming for, so I split it once.
imp.session_state_label.set_text("Shortcut list invalid"); | ||
imp.response_group.set_visible(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, please use the notifications bits that are part of PortalPage already and drop the session_state_label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
let Ok(activated_stream) = global_shortcuts.receive_activated().await | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly broken formatting but why not just write global_shortcuts.receive_activated().await?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot use the
?operator in an async block that returns
()``.
let (abort_handle, abort_registration) = AbortHandle::new_pair(); | ||
let future = Abortable::new( | ||
async { | ||
enum Event { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define this enum outside of this function please
let triggers = self.imp().triggers.lock().await.clone(); | ||
let text: Vec<String> = triggers.into_iter() | ||
.map(|RegisteredShortcut { id, activation }| { | ||
let escape = |s: &str| s.replace("<", "<").replace(">", ">"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure glib has a proper function for this :)
<object class="AdwActionRow"> | ||
<property name="title" translatable="yes">Shortcuts Status</property> | ||
<child> | ||
<object class="GtkLabel" id="shortcuts_status_label"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be unused?
</object> | ||
</child> | ||
<child> | ||
<object class="AdwPreferencesGroup" id="response_group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The respnose group seems to be completely unused, I would suggest moving the binding_changes_count there and maybe adding a row that displays the shortcut that was triggered or something like that
let triggers: Vec<_> | ||
= resp.shortcuts().iter() | ||
.map(|s: &Shortcut| RegisteredShortcut { | ||
id: s.id().into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as elsewhere, use to_owned please
imp.session_state_label.set_text( | ||
&match &response { | ||
Ok(_) => "OK".into(), | ||
Err(ashpd::Error::Response(ResponseError::Cancelled)) => "Cancelled".into(), | ||
Err(ashpd::Error::Response(ResponseError::Other)) => "Other response error".into(), | ||
Err(e) => format!("{}", e), | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, use the notifications infrastructure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make the notifications stay? By the time I get back from the debugger, the message is already gone and I don't know what went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a good point, but there isn't actually. I can change the notification widget implementation separately later :)
ashpd-demo/Cargo.toml
Outdated
@@ -7,7 +7,7 @@ version = "0.4.1" | |||
[dependencies] | |||
adw = {version = "0.6", package = "libadwaita", features = ["v1_4"]} | |||
anyhow = "1.0" | |||
ashpd = {version = "^0.8", features = ["gtk4", "tracing", "pipewire"]} | |||
ashpd = {path="..", features = ["gtk4", "tracing", "pipewire"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work for flatpak, you will have to point it to your github branch until the PR is approved and then merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh.
I don't really know what's happening with the pipeline failure. It's not in globalshortcuts. |
Tested on my GNOME work (not upstreamed yet): https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/2485 https://gitlab.gnome.org/GNOME/gnome-shell/-/merge_requests/3343 https://gitlab.gnome.org/dcz/gsimpl