Skip to content

Commit 227e434

Browse files
InterLinked1InterLinked1
committed
mod_webmail: Prioritize RECENT instead of EXISTS when idling.
Previously, we were mainly looking at untagged EXISTS responses during IDLE to determine whether to send a new message notification/ preview, but this was not ideal as it allowed for many false positives. We now use untagged RECENT responses as our main indicator. This is more reliable, since whenever there is a new message in a folder, the untagged EXISTS is always accompanied by an untagged RECENT anyways. * This avoids scenarios in which an untagged EXISTS is sent by the server, even if there are no new messages in the mailbox. For example, Microsoft servers do this after an untagged EXPUNGE is sent, when a message is moved out of the folder to a different one. Previously, we would send a false positive notification for the latest message in the original folder when this happened. * When we send a RECENT payload to the frontend, include the message's flags. This allows the frontend to know if the message is seen or not, and increment (or not) the folder's unseen count accordingly.
1 parent 24e4b76 commit 227e434

File tree

1 file changed

+98
-48
lines changed

1 file changed

+98
-48
lines changed

‎modules/mod_webmail.c‎

Lines changed: 98 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1886,6 +1886,49 @@ static int contains_attachments(struct mailimap_body *imap_body)
18861886
return 0;
18871887
}
18881888

1889+
static void json_append_flags(struct json_t *msgitem, struct mailimap_msg_att_item *item)
1890+
{
1891+
struct mailimap_msg_att_dynamic *dynamic = item->att_data.att_dyn;
1892+
clistiter *dcur;
1893+
json_t *flagsarr = json_array();
1894+
json_object_set_new(msgitem, "flags", flagsarr);
1895+
if (dynamic && dynamic->att_list) {
1896+
for (dcur = clist_begin(dynamic->att_list); dcur; dcur = clist_next(dcur)) {
1897+
struct mailimap_flag_fetch *flag = clist_content(dcur);
1898+
switch (flag->fl_type) {
1899+
case MAILIMAP_FLAG_FETCH_RECENT:
1900+
json_array_append_new(flagsarr, json_string("\\Recent"));
1901+
break;
1902+
case MAILIMAP_FLAG_FETCH_OTHER:
1903+
switch (flag->fl_flag->fl_type) {
1904+
case MAILIMAP_FLAG_ANSWERED:
1905+
json_array_append_new(flagsarr, json_string("\\Answered"));
1906+
break;
1907+
case MAILIMAP_FLAG_FLAGGED:
1908+
json_array_append_new(flagsarr, json_string("\\Flagged"));
1909+
break;
1910+
case MAILIMAP_FLAG_DELETED:
1911+
json_array_append_new(flagsarr, json_string("\\Deleted"));
1912+
break;
1913+
case MAILIMAP_FLAG_SEEN:
1914+
json_array_append_new(flagsarr, json_string("\\Seen"));
1915+
break;
1916+
case MAILIMAP_FLAG_DRAFT:
1917+
json_array_append_new(flagsarr, json_string("\\Draft"));
1918+
break;
1919+
case MAILIMAP_FLAG_KEYWORD:
1920+
json_array_append_new(flagsarr, json_string(flag->fl_flag->fl_data.fl_keyword));
1921+
break;
1922+
case MAILIMAP_FLAG_EXTENSION:
1923+
json_array_append_new(flagsarr, json_string(flag->fl_flag->fl_data.fl_extension));
1924+
break;
1925+
}
1926+
break;
1927+
}
1928+
}
1929+
}
1930+
}
1931+
18891932
static void fetchlist_single(struct mailimap_msg_att *msg_att, json_t *arr)
18901933
{
18911934
json_t *msgitem;
@@ -1945,45 +1988,7 @@ static void fetchlist_single(struct mailimap_msg_att *msg_att, json_t *arr)
19451988
break;
19461989
}
19471990
} else {
1948-
struct mailimap_msg_att_dynamic *dynamic = item->att_data.att_dyn;
1949-
clistiter *dcur;
1950-
json_t *flagsarr = json_array();
1951-
json_object_set_new(msgitem, "flags", flagsarr);
1952-
if (dynamic && dynamic->att_list) {
1953-
for (dcur = clist_begin(dynamic->att_list); dcur; dcur = clist_next(dcur)) {
1954-
struct mailimap_flag_fetch *flag = clist_content(dcur);
1955-
switch (flag->fl_type) {
1956-
case MAILIMAP_FLAG_FETCH_RECENT:
1957-
json_array_append_new(flagsarr, json_string("\\Recent"));
1958-
break;
1959-
case MAILIMAP_FLAG_FETCH_OTHER:
1960-
switch (flag->fl_flag->fl_type) {
1961-
case MAILIMAP_FLAG_ANSWERED:
1962-
json_array_append_new(flagsarr, json_string("\\Answered"));
1963-
break;
1964-
case MAILIMAP_FLAG_FLAGGED:
1965-
json_array_append_new(flagsarr, json_string("\\Flagged"));
1966-
break;
1967-
case MAILIMAP_FLAG_DELETED:
1968-
json_array_append_new(flagsarr, json_string("\\Deleted"));
1969-
break;
1970-
case MAILIMAP_FLAG_SEEN:
1971-
json_array_append_new(flagsarr, json_string("\\Seen"));
1972-
break;
1973-
case MAILIMAP_FLAG_DRAFT:
1974-
json_array_append_new(flagsarr, json_string("\\Draft"));
1975-
break;
1976-
case MAILIMAP_FLAG_KEYWORD:
1977-
json_array_append_new(flagsarr, json_string(flag->fl_flag->fl_data.fl_keyword));
1978-
break;
1979-
case MAILIMAP_FLAG_EXTENSION:
1980-
json_array_append_new(flagsarr, json_string(flag->fl_flag->fl_data.fl_extension));
1981-
break;
1982-
}
1983-
break;
1984-
}
1985-
}
1986-
}
1991+
json_append_flags(msgitem, item);
19871992
}
19881993
}
19891994
}
@@ -3018,6 +3023,8 @@ static void send_preview(struct ws_session *ws, struct imap_client *client, uint
30183023
fetch_att = mailimap_fetch_att_new_uid();
30193024
mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att);
30203025

3026+
mailimap_fetch_type_new_fetch_att_list_add(fetch_type, mailimap_fetch_att_new_flags()); /* Flags, so we know if this RECENT message is \Seen or not */
3027+
30213028
/* Do NOT automark as seen */
30223029
fetch_att = mailimap_fetch_att_new_rfc822_header();
30233030
mailimap_fetch_type_new_fetch_att_list_add(fetch_type, fetch_att);
@@ -3032,7 +3039,7 @@ static void send_preview(struct ws_session *ws, struct imap_client *client, uint
30323039
goto cleanup;
30333040
}
30343041

3035-
json_object_set_new(root, "response", json_string("EXISTS"));
3042+
json_object_set_new(root, "response", json_string("RECENT"));
30363043

30373044
cur = clist_begin(fetch_result);
30383045
msg_att = clist_content(cur);
@@ -3067,6 +3074,11 @@ static void send_preview(struct ws_session *ws, struct imap_client *client, uint
30673074
default:
30683075
bbs_warning("Unhandled type\n");
30693076
}
3077+
} else {
3078+
/* Include flags in preview. The important one is \Seen, so the frontend
3079+
* knows whether to increment the unread count or not.
3080+
* Of course, this math is only accurate if one message is processed at a time... */
3081+
json_append_flags(root, item);
30703082
}
30713083
}
30723084

@@ -3681,15 +3693,42 @@ static int process_idle(struct imap_client *client, char *s)
36813693
/* What we do next depends on what the untagged response is */
36823694
if (STARTS_WITH(tmp, "EXISTS")) {
36833695
uint32_t previewseqno = (uint32_t) seqno;
3696+
if (previewseqno <= client->messages) {
3697+
/* Microsoft sends an untagged EXISTS after an untagged EXPUNGE when we move a message to a different folder.
3698+
* There aren't actually any new messages in this case. */
3699+
bbs_debug(1, "I thought we had %u messages in this mailbox, and server said we have %u?\n", client->messages, previewseqno);
3700+
}
36843701
client->messages = previewseqno; /* Update number of messages in this mailbox */
36853702
client->idlerefresh |= IDLE_REFRESH_EXISTS;
3703+
/*! \todo We should fetch the flags of all the messages for which we get an untagged EXISTS.
3704+
* This way, we know which ones are \Seen or not, and we can tell the frontend
3705+
* how many new UNSEEN messages there are, not just how many new.
3706+
* When we send a preview for the one most RECENT message, we indicate \Seen or not for that,
3707+
* but for multiple messages, our heuristics may not be accurate. In particular,
3708+
* the case of moving multiple messages into this folder, where at least one of the set outside of the most recent,
3709+
* has the \Seen flag.
3710+
*
3711+
* The downside is this slows things down a bit since we'd now need to issue a new FETCH every time we get untagged EXISTS.
3712+
* Then again, we already do that for refreshing the page (listing of messages). */
36863713
} else if (STARTS_WITH(tmp, "RECENT")) {
3687-
/* RECENT is basically always accompanied by EXISTS, so this is almost academic,
3688-
* since we don't currently use this flag for anything, but for sake of completeness: */
3689-
/*! \todo We should use this, because not all EXISTS are accompanied by EXISTS.
3690-
* For example, if a seen message is copied to this folder.
3691-
* We should provide explicit information to the client about whether this
3692-
* message is seen or not, using this information. */
3714+
/* RECENT indicates a brand new message in the mailbox.
3715+
* We should only alert about new messages if it's RECENT.
3716+
* This helps ensure we avoid false positives, e.g:
3717+
* - Microsoft servers will send an untagged EXISTS along with EXPUNGE
3718+
* when a message gets moved out to another folder. There are no new
3719+
* messages, to the EXISTS should be ignored (not trigger a notification).
3720+
* There is no RECENT in this case, so never sending notifications
3721+
* for EXISTS unaccompanied by RECENT ensures we don't do this.
3722+
* - A seen message is copied to this mailbox. This will trigger an EXISTS,
3723+
* and also a RECENT, even though the message has the \Seen flag.
3724+
* - If a message is marked as unread in the same mailbox, it should trigger
3725+
* an untagged FETCH only. This should never trigger a notification.
3726+
*
3727+
* The main false positive that could still occur is when a message is moved
3728+
* to this folder from another mailbox. However, standard mail clients
3729+
* will also do this, so this is not a huge issue. We could try to suppress
3730+
* notifications for messages we think were already in the account by looking
3731+
* at the INTERNALDATE, but this isn't foolproof either. */
36933732
client->idlerefresh |= IDLE_REFRESH_RECENT;
36943733
} else if (STARTS_WITH(tmp, "EXPUNGE")) {
36953734
if (client->messages) {
@@ -3805,17 +3844,28 @@ static int on_poll_activity(struct ws_session *ws, void *data)
38053844
if (client->idlerefresh) { /* If there were multiple lines in the IDLE update, batch any updates up and send a single refresh */
38063845
int r = client->idlerefresh;
38073846
/* EXPUNGE takes priority, since it involves a mailbox shrinking */
3808-
const char *reason = r & IDLE_REFRESH_EXPUNGE ? "EXPUNGE" : r & IDLE_REFRESH_EXISTS ? "EXISTS" : r & IDLE_REFRESH_FETCH ? "FETCH" : "";
3847+
const char *reason = r & IDLE_REFRESH_EXPUNGE ? "EXPUNGE" : r & IDLE_REFRESH_RECENT ? "RECENT" : r & IDLE_REFRESH_EXISTS ? "EXISTS" : r & IDLE_REFRESH_FETCH ? "FETCH" : "";
38093848
/* In our case, since we're webmail, we can cheat a little and just refresh the current listing.
38103849
* The nice thing is this handles both EXISTS and EXPUNGE responses just fine. */
38113850
idle_stop(ws, client);
38123851

3852+
if (r & IDLE_REFRESH_EXPUNGE) {
3853+
/* The frontend relies on us to tell it how many message, unseen and total, exist following any EXPUNGE in the mailbox. */
3854+
uint32_t num_total = 0, num_unseen = 0;
3855+
res = client_status_basic(client->imap, client->mailbox, &num_unseen, &num_total);
3856+
client->messages = num_total;
3857+
client->unseen = num_unseen;
3858+
}
3859+
38133860
REFRESH_LISTING(reason);
3814-
if (r & IDLE_REFRESH_EXISTS) {
3861+
/* Only counts as a new message if it was RECENT.
3862+
* However, we can only infer the sequence number from the untagged EXISTS. */
3863+
if ((r & IDLE_REFRESH_RECENT) && (r & IDLE_REFRESH_EXISTS)) {
38153864
/* Send the metadata for message with this sequence number as an unsolicited EXISTS.
38163865
* It's probably this message, assuming there's only 1 more message.
38173866
* (In the unlikely case there's more than one, we wouldn't want to show multiple notifications anyways, just one suffices.)
38183867
* Do NOT automark as seen. This is not a FETCH. */
3868+
bbs_debug(5, "Sending preview of %s, seqno %u\n", client->mailbox, client->messages);
38193869
send_preview(ws, client, client->messages);
38203870
}
38213871
client->idlerefresh = 0;

0 commit comments

Comments
 (0)