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

Bubble: minor refactor #202

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Bubble: minor refactor #202

merged 3 commits into from
Jun 28, 2023

Conversation

Marukesu
Copy link
Contributor

remove the replace() method and construct block in favor of construct the contents widget in one place. we also stop checking if the notification has no action for launching the application, instead we launch it whenever we don't have a default action.

Fixes some cases of #195/#145.
Fixes: #170

@Marukesu Marukesu requested a review from a team June 24, 2023 19:00
src/Bubble.vala Outdated
dismiss ();
});
protected override bool leave_notify_event (Gdk.EventCrossing event) {
if (notification.priority < GLib.NotificationPriority.HIGH) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't there a change recently with notifications because we treat high priority as a normal priority? I would think this is supposed to be less than urgent is that is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's #189, i prefer to keep PRs the most self-contained possible, so that they can be reviewed and merged in any order.

@danirabbit
Copy link
Member

@Marukesu can you resolve conflicts here please?

@danirabbit danirabbit mentioned this pull request Jun 27, 2023
Marukesu added 3 commits June 27, 2023 17:28
Signed-off-by: Gustavo Marques <[email protected]>
DRY the content creation code by handling it in the setter for the
notification property.

Signed-off-by: Gustavo Marques <[email protected]>
@Marukesu Marukesu force-pushed the maru/bubble-fix-replace-contents branch from 93d84b5 to 8cb9676 Compare June 27, 2023 20:49
@Marukesu
Copy link
Contributor Author

@danirabbit done.

Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

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

Code looks good and simpler as well. Fixes the linked issue. Good job!

@lenemter lenemter merged commit e73af6d into master Jun 28, 2023
@lenemter lenemter deleted the maru/bubble-fix-replace-contents branch June 28, 2023 16:56
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.

Urgent notifications automatically dismissed when being replaced
4 participants