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

Background: Make Asynchronous #84

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Background: Make Asynchronous #84

merged 2 commits into from
Jun 12, 2023

Conversation

Marukesu
Copy link
Contributor

@Marukesu Marukesu commented Jun 7, 2023

Make all of the DBus calls and methods asynchronous so that we don't end up blocking others interfaces, methods and initialization. The notifications server proxy is now started when we first call send_notification() so we don't end starting it early than necessary.

@danirabbit danirabbit requested a review from a team June 8, 2023 00:43
notification.response (NotifyBackgroundResult.CANCELLED);
if (reason == CANCELLED) { // Closed via DBus call
notification.response (CANCELLED);
} else { // Dismissed, Expired, or something internal to the server
Copy link
Member

Choose a reason for hiding this comment

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

A expired signal is also sent when the notification bubble closes. I think it's better to keep waiting then because the notification will stay in the indicator and can still send a response when something like elementary/wingpanel-indicator-notifications#258 gets merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This's a notification server bug, currently the server and indicator works at the back of each other, making the integration between both a mess. I'm working in a "rewrite/refactor" of the server right now, so that we can support the indicator properly.

we can wait till the server is fixed, but i don't see why, because the expiration would make the action activation useless in the server side. and we would need to fix that before something like elementary/wingpanel-indicator-notifications#258 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Only out of curiosity: Why would the expiration make the action activation useless even if we ignore the expiration signal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the server emits a NotificationClosed signal it's mean that the notification doesn't exists anymore. so there will be no other signal emission for that notification id from the server.

src/Background/NotificationRequest.vala Outdated Show resolved Hide resolved
src/Background/Portal.vala Show resolved Hide resolved
}

var path = Path.build_filename (Environment.get_user_config_dir (), "autostart", _app_id + ".desktop");

var file = File.new_build_filename (Environment.get_user_config_dir (), "autostart", _app_id + ".desktop");
Copy link
Member

Choose a reason for hiding this comment

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

Only out of curiosity: What's the advantage here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GLib recommends that we should use the asynchronous versions of the methods in asynchronous code. In this case, it's reduce the chances that we slow or block others methods and others portals.

src/Background/Portal.vala Outdated Show resolved Hide resolved
* move dbus interfaces out of the classes bodies, use constants for path and
  name.

* use enumeration values where applicable and skip the namespace when possible.

* simplify the response callback in the notify_background().

* make the notification_closed() callback return ALLOW_ONCE in more types of
  reasons.

* remove temporary variables where applicable.

Signed-off-by: Gustavo Marques <[email protected]>
make all of the dbus calls and methods asynchronous so that we don't end up
blocking others interfaces, methods and initialization.

the notification proxy is now also started when we first call
send_notification() so we don't end starting it early than necessary or
blocking the initialization for too much time.

Signed-off-by: Gustavo Marques <[email protected]>
Copy link
Member

@leolost2605 leolost2605 left a comment

Choose a reason for hiding this comment

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

Works as it should and LGTM :)

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.

3 participants