-
Notifications
You must be signed in to change notification settings - Fork 32
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
Close libnotifyplugin notifications when message are read #208
Conversation
I tested all four libnotifyplugin notification options. But I didn't test whether the additional calls to MAILS_REMOVED hooks cause any problems for other plugins or extensions, as my desktop environment doesn't support them. |
Let me know if there is anything you'd like me to change on this pull request. |
Hey @jdchristensen, many thanks for your pull request! Your fix is very appreciated I just didn't find the time for a in-depth review yet. I'll try to have a closer look within the next feew days :) |
Merged, thanks again! |
Yes, you're absolutely right. Actually I am the one who was confused - I also checked whether your changes affect the mailnag gnome-shell extension. This extension retrieves copies of mail objects via dbus, so holding references there would unnecessarily double the ram consumption for mail objects. Since your references are stored within the mailnag process itself and notifications are purged as soon as those mails are gone, there's of course no mem overhead. Nevermind, the "leak" you encountered is probably due to some python runtime optimization :)
Thanks again for your contribution!
BR
Patrick
Am 28. Juni 2020 20:14:38 MESZ schrieb Dan Christensen <notifications@github.com>:
…
@jdchristensen commented on this pull request.
> n = self._get_notification(self._get_sender(mail), mail.subject,
"mail-unread")
+ # Remember the associated message, so we know when to remove the
notification:
+ n.mail = mail
Unless I'm confused, that line is just storing a reference to the mail
object, not the object itself, so it should be fast and not add to the
memory consumption. (The notification should be closed as soon as we
learn that the message is gone, and once it is closed, the mail object
should be garbage collected.) Storing just the mail id would make the
line that deletes the notifications a bit more complicated, but I can
do it if you like. I don't know how equality of mail objects works,
but it could be every-so-slightly faster to compare their ids instead
of the full objects. Seems unlikely to be worth the extra complexity.
I did some tests where I created and deleted 10 messages per second for
a while. I'm not 100% sure, but it does appear that there might be a
memory leak somewhere in the code. But the memory usage grows even if
I change _notify_single to be a noop, so if there is a leak, I think it
is elsewhere. VSZ stays constant but RSS grows slowly, so maybe this
isn't a leak, but just the nature of how the memory management works?
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
#208 (comment)
--
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
|
In 'single notification per message' mode, when a message is read, the corresponding notification is closed.
In all other modes, the notification is closed when there are no unread messages, and it is updated when there are new unread messages. It is not changed when the only change is that some messages have been marked read, as this would cause the notification to pop up again.
Closes #37