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

Remove notification from list if bubble dismissed; give accurate tooltip count #243

Merged
merged 7 commits into from
May 21, 2023

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Mar 6, 2022

At present if a notification bubble is actively dismissed it remains in the wingpanel list and has to be dismissed again to get rid of it. This seems unexpected. This PR removes notifications from the list if they are actively closed by the user or a "CloseNotification" method call.

It is open to question whether this should also happen in reverse i.e. urgent bubbles should disappear if cleared from the wingpanel list (at present they do not) but that is left for another PR (after hiding bubbles when the indicator is open is implemented).

There was existing code in the NotificationsMonitor that appeared to be intended to implement this already but it did not detect NotificationClosed signals.

A superfluous function/signal was removed from Notification; the Indicator detects when a notification is closed.

@TomiOhl
Copy link
Contributor

TomiOhl commented May 15, 2023

Bumping this - any reason this PR is abandoned? Seems to be a welcome change based on the 🎉 reactions

@jeremypw jeremypw requested a review from a team May 16, 2023 10:03
@jeremypw
Copy link
Collaborator Author

jeremypw commented May 16, 2023

Re-testing this as it is old. Notice that the indicator tooltip gives an inaccurate notification count - checking if occurs in master. Yes, it does.

@jeremypw jeremypw changed the title Do not keep notification in list if bubble dismissed Remove notification from list if bubble dismissed; give accurate tooltip count May 16, 2023
@jeremypw
Copy link
Collaborator Author

I have fixed the tooltip count here, but that could be split out if necessary.

leolost2605
leolost2605 previously approved these changes May 17, 2023
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.

Only one small question otherwise it looks good to me and can confirm it works as intended. Regarding splitting the fix i wouldn't deem it necessary but that's just my opinion.

src/Indicator.vala Show resolved Hide resolved
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.

Works perfectly 👍

@lenemter lenemter merged commit da9220f into master May 21, 2023
@lenemter lenemter deleted the remove-dismissed-notifications branch May 21, 2023 07:51
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.

6 participants