node-imapnotify icon indicating copy to clipboard operation
node-imapnotify copied to clipboard

Proposed PR: trigger script on delete/update events

Open rlue opened this issue 7 years ago • 5 comments

First of all, just want to say thank you for a very handy utility.

I wanted mbsync to run not just when I received new mail, but also whenever messages were deleted from the server, or marked as read (in case, for example, I delete them from my phone). For that reason, I added event listeners for node-imap's 'expunge' and 'update' events. (It's not very pretty, I know – JS is not my main language.)

But since that functionality goes beyond the stated purpose of imapnotify ('Execute scripts on new messages using IDLE IMAP command'), I thought it would make more sense to hold off on a PR, and ask if there would be interest in merging this feature first.

Are there any use cases where users would want to trigger their scripts only on new mail, and not on deletions or other changes to the mailbox?

rlue avatar May 01 '17 13:05 rlue

Hi, thanks for stopping by. I would gladly accept such a pull request, you are welcome to contribute.

a-sk avatar May 02 '17 09:05 a-sk

In my case i would like to only receive notifies on new mail. I think the way forward is to keep onNotify/onNotifyPost as the catch all and create a few new configuration options:

  • onNewMail (only for new mail)
  • onNewMailPost (only for new mail)
  • onExpunge (only for expunge)
  • onExpungePost (only for expunge)
  • onUpdate (only for update)
  • onUpdatePost (only for update)

That way someone can configure onNotify to run mbsync (so it syncs on new, expunge and update) and onNewMailPost to run a notification in the desktop (so it alerts only new email).

Be alerted of deleted messages makes no sense in the current situation since the user in the vast majority of cases is the one who deleted it in the first place.

renatofdds avatar May 03 '17 20:05 renatofdds

That's originally how I had wanted to do it, but 1) JS is not my main language, and 2) I didn't want to spend the time figuring it out if people weren't going to use it. I had not considered the desktop notifications use case.

I'm happy to give it a try, but me fooling around with all these callbacks is likely to get ugly before it gets pretty again.

@a-sk, thoughts? I'd be fine with you rolling back the changes, if that's what your other users want – I can rely on my own fork, in the meantime.

rlue avatar May 04 '17 03:05 rlue

No problem @rlue , thank you for your contribution! I'll code it when i find some time in the next few days and I'll submit a PR.

renatofdds avatar May 04 '17 17:05 renatofdds

This does seem to be implemented now. @a-sk close issue?

rasendubi avatar Dec 18 '17 16:12 rasendubi