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
System notification via event notification system #3429
Conversation
@flatcap this is my first attempt at making this work. Looks to be working, but let me know if this is the right direction. I'll try to add some tests soon. Also, can you help me figure out how to document it in the manual? |
Yep. This a great start! Well done!
Some format strings refer to Lines 2575 to 2581 in d5b865c
Some others explicitly list their expandos: Lines 2521 to 2536 in d5b865c
Note: I've made the Pull Request into a "draft" while you develop -- it reduces the load on GitHub |
Thanks, need to remember this for the next time. |
de9641b
to
1976ce5
Compare
Added some changes. I was wondering, would it be correct to check for new (and not notified) messages using |
Yes. And then some :-)
Hmm... I hadn't thought of that.
Yes, so we'll need some storage for that data. Or we could have a new mail struct in
No. This is the big as-yet-unresolved problem in NeoMutt. The GUI and the backends share an array of Emails. Every so often, the event loop in the Index or Pager will call For efficiency, the Emails now get resorted:
The new Emails are added to the end of the array.
Yes, but I'm not expecting you to implement it :-) My plan is that the GUI and backends both have an array of Emails. When new mail arrives, the backend knows which Emails are new (and updates its array). This fits in nicely with the new-mail command -- it's given just the information it needs. |
Next StepsQuite a lot's changed since yesterday! The biggest change is that you've moved the code to I suggest creating a new library Lines 542 to 557 in d5b865c
Also, can you add a Lines 123 to 132 in d5b865c
You'll need an entry, here: Lines 154 to 163 in d5b865c
And you'll want to make your own copy of: Lines 1219 to 1222 in d5b865c
Then you can wrap your code in |
Hmm, that complicates things, checking each time for which messages are new could be terribly inefficient. I suppose this could be fixed by sending notification event in the backends. I haven't looked at that code very closely, but I'm assuming there must be a moment in time when the backend knows when it's discovering a new message. There's some other questions to consider, that I'm now just thinking of:
|
Yes, I moved it to test. I'll create new lib as suggested, makes sense. Btw, I actually wanted to test a bit more functionality, including the expandos, but I see it's not that easy -- I don't suppose we can link that code currently. And by the look of it, it's not trivial to move to a lib (at least not on its own). I suspect this is a known limitation? Any plans to restructure the code to allow for better testing in the future? |
All the structures are in-memory, so it probably won't be that slow.
Yes, that's where we need to go, but your Lines 186 to 189 in d5b865c
Yes, somewhere in
Not sure. The new-mail command is to notify you when NeoMutt is unattended. Sounds like a config option, but not yet :-)
We could have expandos for both and let the user decide:
This means we'd need to keep track.
Upstream added a couple of flags to the
Implementing them would probably be enough control. |
hehe, yeah, it's a ****ing nightmare!
Correct.
I created a proof-of-concept for fixing the expandos: https://github.com/neomutt/test-expando Like my refactoring of the Implementing that would be hard, but would make a lot tricky code, testable. |
I suppose. Feels wasteful, but I get your point about a temporary solution.
Makes sense.
Ok, I'll look into that possibly after this PR.
Yep, from my very preliminary look at the code, it looks non-trivial. BTW. I've been testing this build, and I ran into a problem where I kept |
@flatcap I created a new optional lib, but I think I'm doing something wrong. I don't seem to compile my tests when i define the flag, though Is there anything obvious I'm doing wrong? |
Rebased and fixed (minor stuff, see commit messages for details). The one thing you were missing was the hardest to find... Lines 1300 to 1305 in 0cf1981
Rather than export |
Ah, thanks for giving it a look! |
@flatcap I was just looking into storing last time of notification... these must be per mailbox, right? to work correctly... The question is: how to store these? Should I create some structure on the newmail side or another field in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first checkpoint should be using realistic notifications.
const char *if_str, const char *else_str, | ||
intptr_t data, MuttFormatFlags flags); | ||
int handle_new_mail_event(const char *cmd, struct NotifyCallback *nc, Execute *execute); | ||
int new_mail_observer(struct NotifyCallback *nc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only new_mail_observer()
is used from the outside, so we can drop everything else.
Including the #include
s. They could be replaced with a forward declaration:
struct NotifyCallback;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually not entirely true. I am using those functions in tests. I split them to be able to write some unit tests. If I merge all of them together, it will be very difficult to write any tests. I suppose you can hack it and set config to use some specific command that writes to a file and we then read it to verify... Would you prefer I did that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I always forget the tests.
Leave them as they are and I'll re-educate myself on their usage :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; if you see a better way to test these, let me know.
if (!c_devel_new_mail_command) | ||
return 0; | ||
return handle_new_mail_event(c_devel_new_mail_command, nc, execute_cmd); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I'd merge handle_new_mail_event()
and execute_cmd()
into new_mail_observer()
.
We don't need the extra levels of indirection or swap-out-able-ness, yet.
mx.c
Outdated
struct EventMailbox ev_m = { m }; | ||
notify_send(m->notify, NT_MAILBOX, NT_MAILBOX_NEW_MAIL, &ev_m); | ||
m->notified = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want this to be a step up from the old feature...
I suggest a simple mechanism to start:
- Create a
static time_t
in this file that stores the time of the last check - The first time through this code,
(t == 0)
, we update the time to now, but don't send a notification - Next time, we search the
Mailbox
for anyEmail
s whosereceived
time is aftert
and put them in an array - Send a notification with the
Mailbox
and theEmailArray
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say EmailArray
do you mean an existing type? I do see MdEmailArray
but that seems to be something different. Should I define a similar one for the purpose of the notifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmailArray
do you mean an existing type?
No, it'll be a new type using ARRAY
(which works like a C++ template).
We want an ARRAY
of struct Email *
(similar to MdEmailArray
).
As it will be general-purpose, we can put it at the end of the struct Email
definition.
(Note: ARRAY
supports creating arrays of objects, e.g. AliasViewArray
)
We'll use the ARRAY
as a temporary collection.
Since the Email
s will be owned by the Mailbox
we only need to ARRAY_FREE()
afterwards.
We already do something similar in the Index, op_pipe()
.
Unfortunately, that was written before we'd invented ARRAY
.
Lines 1763 to 1766 in ef0cb55
struct EmailList el = STAILQ_HEAD_INITIALIZER(el); | |
el_add_tagged(&el, shared->mailboxview, shared->email, priv->tag); | |
mutt_pipe_message(shared->mailbox, &el); | |
emaillist_clear(&el); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for clarifying.
Yes,
There's also a "virtual" array,
|
Thanks for the explanation. I pushed some changes, and I started testing it after changes to see if it works as expected. I guess there's still a question of expandos to use. I've been using some random characters I came up with, but let me know if you'd like me to change these. I know you mentioned something about a two-character |
I must be missing some important point here... I kept getting a segfault on access to an email, so I debugged it, and I see some of those emails in range 0 to Not to mention that some of them are actually What am I missing? |
That's fine for now
It's a bit of a cheat. Lines 1220 to 1221 in ef0cb55
|
Hmm... don't know, they should be valid
Hmm... I nearly mentioned this.
I'll give it a try to see if I can figure it out. |
I forgot to mention a "feature" of struct Email *e = email_new();
ARRAY_ADD(ea, e);
// ...
struct Email **ptr = ARRAY_GET(ea, 0);
struct Email *e2 = *ptr; This seems like a pain, but it's a feature. if (!ptr) // array index was out of bounds
struct Email *e = *ptr;
if (!e) // array element is `NULL` |
I am at a loss here. Here's my test: (1) starting with empty gmail Inbox, (2) sending an email, (3) running neomutt with some debug messages printing: I get the correct number of messages and unread messages, but all of the pointers are NULL. Here's the function I use for this test: void find_new_emails(const struct Mailbox *mailbox, time_t last_notified,
struct EmailArray *emails)
{
int i = 0;
mutt_debug(LL_DEBUG1, "Mailbox: %s\n", mailbox_path(mailbox));
mutt_debug(LL_DEBUG1, "MSG COUNT: %d\n", mailbox->msg_count);
mutt_debug(LL_DEBUG1, "Unread: %d\n", mailbox->msg_unread);
for (; i < mailbox->msg_count; i++)
{
struct Email* email = mailbox->emails[i];
mutt_debug(LL_DEBUG1, "msg(%d): %d\n", i, email);
}
} And here's the output:
No matter what I do: go to that inbox, read some emails, etc. -- just all are zeroes. I am perplexed. No idea what I'm doing wrong here. |
I've just made some small tweaks...
With the callback in |
@flatcap I made some changes, and I think this is currently working as I expect it to. I'm running it for my own workflow to find any potential issues. A few comments: I found that defining two additional fields for a mailbox simplifies the code a lot, so I went for that. We can always consider refactoring that, but at the moment, it's the cleanest approach, I think. You'll notice that I update the unnotified counter (btw. can you think of a better name?) in a different place and under different conditions than I wasn't sure how to approach unread emails count with regards to I'm setting the initial time to 0, which means we'll be notified on startup about any new messages. I gave it some thought and I think this actually makes sense, especially for maildir, where new emails can come without neomutt being open. So it's not such a bad idea to notify if there are any. Though ideally, we'd probably want a config option for that to allow the user to decide what they want. What do you think? Finally, as I said before, I only focused on maildir. I think we should merge maildir first; the question is whether to continue working on maildir functionality, or first implement the existing functionality for other backends? |
be9aee4
to
758d1eb
Compare
a022aea
to
5dfc1a0
Compare
Introduces `devel_new_mail_command` config option. When used instead of `new_mail_command`, Neomutt's event notification system will be used for new mail notifications. This is part of a bigger refactoring of the legacy notification approach. The initial implementation uses only very limited set of expandos.
Rename to match library dir
Not all defines in auto.def are exported. See auto.def:1300
Archived to: https://github.com/neomutt/neomutt-old/tree/notify |
Introduces
devel_new_mail_command
config option. When used instead ofnew_mail_command
, Neomutt's event notification system will be used fornew mail notifications. This is part of a bigger refactoring of the
legacy notification approach. The initial implementation uses only very
limited set of expandos.