Skip to content
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

newmail_notifier: Include sender and subject in desktop notification #9492

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Daniel15
Copy link

Displays the sender and subject in desktop notifications:
image

References #8360 #7826 #8587

Comment on lines 48 to 50
? ' +' +otherCount
// No sender name, e.g. "New Mail! (3)
: '('+otherCount + ')';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add spaces around the plus-signs. Or maybe you could rework these title-related lines a little, including line 40, to be a little less terse but more readable?

Copy link
Member

@pabzm pabzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, I like it!

From my side there's only the minor nitpick, the rest looks good!

if ($latest_uid !== null) {
$headers = $storage->fetch_headers($mbox, [$latest_uid]);
// fetch_headers returns an array, but we only care about the first one.
$headers = array_shift($headers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but here you could use array_first().

$headers = array_shift($headers);
if ($headers !== null) {
$from = $headers->from;
$subject = $headers->subject;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what sanity checks we should apply here. We probably should not use too long subjects. Also we should parse the From header content. We definitely should decode the content, not pass as-is.

See program/actions/mail/show.php for hints on what we normally do with subject and from header.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll take a look. I wasn't sure whether to limit the subject length here or on the client side.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should not use too long subjects.

I tried with a long subject, and KDE makes them scrollable:

image

I haven't tested with Windows yet, but I think it truncates them.

We definitely should decode the content, not pass as-is. See program/actions/mail/show.php for hints on what we normally do with subject and from header.

Thanks for the pointer. I thought the headers would already be decoded here.

var otherCount = prop.count - 1;
if (otherCount >= 1) {
title += prop.from
// Have sender name, e.g. "Daniel L +3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this whole thing. Neither "Daniel L +3" looks good nor "New Mail! (3)".

How about we show the sender/subject only when there's a single new mail message? And "(n) New Mail!" in other cases?

@Daniel15
Copy link
Author

I haven't had free time to revise this yet, but I'll get back to it soon.

@Daniel15
Copy link
Author

Daniel15 commented Jul 8, 2024

Updated to properly decode the from and subject headers. I updated it to just show the sender name rather than their email address too, to make the notification a bit more concise.

Tested with a multi-byte (Chinese) subject:
Screenshot_20240707_175454

Tested with a Chinese sender name:
Screenshot_20240707_175550

 - Show "(2) New Mail!" when there's more than one email
 - Decode from and subject headers

Signed-off-by: Daniel Lo Nigro <git@d.sb>
$from_details = array_first(
rcube_mime::decode_address_list($headers->from, 1, true, $headers->charset)
);
$from = $from_details['name'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name can be empty. In such case let's fallback to the email address. Also, in some corner cases From header might not exist at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants