search

pulb / mailnag

An extensible mail notification daemon
GNU General Public License v2.0
250 stars 32 forks source link

Close libnotifyplugin notifications when message are read #208

Closed jdchristensen closed 4 years ago

jdchristensen commented 4 years ago

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

jdchristensen commented 4 years ago

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.

jdchristensen commented 4 years ago

Let me know if there is anything you'd like me to change on this pull request.

pulb commented 4 years ago

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 :)

pulb commented 4 years ago

Merged, thanks again!

pulb commented 4 years ago

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: https://github.com/pulb/mailnag/pull/208#discussion_r446681147

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.