Closed Bug 1547202 Opened 1 year ago Closed 1 year ago

Blank tray pop-ups of new message notifications for non-Inbox folders

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
normal

Tracking

(thunderbird68+ fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed

People

(Reporter: marcoagpinto, Assigned: jorgk-bmo)

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

The add-ons are broken (Enigmail+ImportExportTool) and when it checks for messages at start, the pop-up tray icon window is blank.

I believe the tray icon window issue happens since beta 66 or 65.

I think this should be: When it checks for messages at start, the pop-up tray icon window is blank.
Some add-ons may be broken in TB 67 beta, that's expected.

Summary: Blank tray pop-ups → Blank tray pop-ups of new message notifications

Hello Alice, could you try finding the regression for us. This one is not so easy. The problem is that the new e-mail notification in the Windows systray (sometimes?) shows up blank.

So I guess the STR could be:
Send yourself an e-mail, quit TB before it gets retrieved.
Start (another version of) TB and watch the new mail notification.

I've noticed blank new mail notification windows when starting various local build, and I've seen it for a while, I'd say at least since the start of 2019.

Flags: needinfo?(alice0775)
Attached image image.png

I cannot reproduce the issue on latest Daily with new profile.
The notification popup seems to be as expected (see screenshot).

Build ID 20190425094146
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.0a1

Flags: needinfo?(alice0775)

(In reply to Alice0775 White from comment #3)

Created attachment 9060963 [details]
image.png

I cannot reproduce the issue on latest Daily with new profile.
The notification popup seems to be as expected (see screenshot).

Build ID 20190425094146
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0)
Gecko/20100101 Thunderbird/68.0a1

Please ignore, Maybe I misunderstood the str.

Well, your screenshot shows a non-empty notification, but I've just received and empty one. I'm still not sure whether that was a SPAM message and the emptiness is expected.

I do not know exact STR and expected reaults.

STR1:

  1. Send a message from another application
  2. Start Thunderbird and select [unrelated folder] (i.e, other than inbox)
  3. Wait for a while

Actual Results and Expected Results:
Sound played. Tray Icon appears. Notification popup appears with out any problem

STR2:

  1. Send a message from another application
  2. Start Thunderbird and select [inbox folder]
  3. Wait for a while

Actual Results:
Good build : Sound played. Tray Icon appears. But no notification popup appears(52esr) or the popup appears and disappears quickly(60esr).
Bad Build: Sound played. Tray Icon appears. Notification popup appears, but subject is blank.

Expected Results:

Regression window
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=d4e98be4fc0b4bbc5f65149e874048b600daf21e&tochange=bd1dfa52d616bce5e6024b8a6c2524f0658e6205
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cfdbecb2e2305dba0ccac931aa91423135039d2f&tochange=5fb87d2312ea4881026ecaac6488fbdc2e4a7725

Thank you so much!!

Flags: needinfo?(mkmelin+mozilla)

On Linux I set "mail.biff.use_system_alert;false" because I wanted to be able to click an email in the notification and have it open in Thunderbird if I was using another application like Firefox or SMPlayer.

Using Thunderbird 67.0b1 and 68.0a1 on Linux I mainly see an empty notification when my Gmail IMAP account gets new mail from Bugzilla. Messages to the Inbox appear in the notification.

Yes, the notifications for new messages in the inbox seem to work, but not for other folders if new messages appear there.

I updated to 67.0b2 on Linux Mint 19 and now I hear my alert sound but don't see any notification at all for mail received in my Bugzilla folder.

I've tested with "mail.biff.use_system_alert;false" and set to true to use native notifications on Linux.

Attached patch bug1547202_blank_new.patch (obsolete) — Splinter Review

Some improvements and bug fix - not sure it fixes this particular case.

Assignee: nobody → mkmelin+mozilla
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Summary: Blank tray pop-ups of new message notifications → Blank tray pop-ups of new message notifications for non-Inbox folders
Comment on attachment 9068000 [details] [diff] [review]
bug1547202_blank_new.patch

Review of attachment 9068000 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/foldersummary.js
@@ +153,1 @@
>              decodeURIComponent(escape(msgHdr.getStringProperty("preview")));

Would it make sense not to evaluate `msgHdr.getProperty("preview")` twice. So do:
let preview;
if (this.showPreview && (preview = msgHdr.getProperty("preview"))) {
Comment on attachment 9068000 [details] [diff] [review]
bug1547202_blank_new.patch

Review of attachment 9068000 [details] [diff] [review]:
-----------------------------------------------------------------

At least the last hunk I'm commenting on here should make a change. I'll do a beta build and start using it, so I'll see whether the issue goes away:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=17709f49c6cbd8d7102b132195564312febf1bb0

::: mail/base/content/foldersummary.js
@@ +110,5 @@
>  
>        foundNewMsg = true;
> +      if (this.children.length >= this.maxMsgHdrsInPopup) {
> +        return foundNewMsg;
> +      }

Does that fix the issue of huge notifications? I though seeing all summaries/previews was quite useful ;-)

@@ +211,5 @@
> +      let node = event.target;
> +      while (node.hasChildNodes()) {
> +        node.lastChild.remove();
> +      }
> +    });

What's this change for?

::: mailnews/base/content/newmailalert.js
@@ +51,5 @@
>    // This is really the root folder and we have to walk through the list to
>    // find the real folder that has new mail in it...:(
>    let allFolders = rootFolder.descendants;
>    var folderSummaryInfoEl = document.getElementById("folderSummaryInfo");
> +  folderSummaryInfoEl.maxMsgHdrsInPopup = gNumNewMsgsToShowInAlert;

Wow, that typo/copy&paste error is pretty fatal since we have:
`for (let i = 0; i < this.maxMsgHdrsInPopup && i < numMsgKeys.value; i++)`.

(In reply to Jorg K (GMT+2) from comment #13)

Does that fix the issue of huge notifications? I though seeing all
summaries/previews was quite useful ;-)

It does yes. Maybe we should indeed increase the number we're showing.

What's this change for?

The event handlers should have been part of the object. It worked earlier, but you'd have to always remember to set the event handler inline in the markup, or break yourself. So better have the object handle them automatically.

Wow, that typo/copy&paste error is pretty fatal since we have:
for (let i = 0; i < this.maxMsgHdrsInPopup && i < numMsgKeys.value; i++).

Yep, I think the changes here are correct and needed, but not necessarily fixing the issue at hand. Maybe we should still get it landed. It shouldn't hurt anything.

Attached patch bug1547202_blank_new.patch (obsolete) — Splinter Review
Attachment #9068000 - Attachment is obsolete: true
Attachment #9068281 - Flags: review?(jorgk)
Comment on attachment 9068281 [details] [diff] [review]
bug1547202_blank_new.patch

Review of attachment 9068281 [details] [diff] [review]:
-----------------------------------------------------------------

I'm running with a beta with this patch. Let's see whether it helps. Given that we're busted, there's no rush. Meanwhile ...

::: mail/base/content/foldersummary.js
@@ +108,5 @@
>          return false;
>        }
>  
>        foundNewMsg = true;
> +      if (this.children.length >= this.maxMsgHdrsInPopup) {

... can you check where this is set and maybe increase it to 5 or so.

I'm running this in the beta now and I when the notifications used to be blank before, they no longer are. So I believe this is fixed.

Made it 6 msg that we show.

Attachment #9068281 - Attachment is obsolete: true
Attachment #9068281 - Flags: review?(jorgk)
Attachment #9068291 - Flags: review?(jorgk)
Comment on attachment 9068291 [details] [diff] [review]
bug1547202_blank_new.patch

OK. However, I cheered too early, I've just received a blank notification again :-(
Attachment #9068291 - Flags: review?(jorgk) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ca6c4600f029
fixes for message numbers in newmailalert.js and foldersummary.js. r=jorgk

Keywords: checkin-needed
Target Milestone: --- → Thunderbird 69.0

Coming to think of it, the alert might have never worked correctly. I think in TB 60 we got notifications like: "XXX received 2 new messages", but the preview of only one message was shown, so the notification only ever had one item and thus a fixed size. De-XBL made it work better and the notification is now bigger according to the number of messages and shows multiple previews.

I have a reliable way to reproduce this: Pop account, move bugmail to a bugmail folder with a filter, comment on bug 1343157 (Invalid bugs).

Mail arrives, filter moves it, notification is blank.

Attached patch debug-patch.patch (obsolete) — Splinter Review

I added some debugging: parseFolder() isn't even called. You can convince yourself that it's working properly by hovering the folder and looking at the tooltip which is produced here:

mail/base/content/foldersummary.js
233 if (tooltipnode.parseFolder(msgFolder, null, asyncResults)) {

With the debug I see:
=== prefillAlertInfo: before let folder of fixIterator
=== prefillAlertInfo: looking at 0 AAA Chat
=== prefillAlertInfo: looking at 0 From Inbox
=== prefillAlertInfo: looking at 00 Raspberry Pi
=== prefillAlertInfo: looking at Good ones
=== prefillAlertInfo: looking at 0000 AAA SMIME
[snip]
followed by a long list of folders that belong to this account I'm checking. The folder where the message was moved do, a local folder which is part of the account wasn't checked.

As I said, I doubt that it ever worked. Maybe in the past, some stale old "new" message was notified (EDIT: or messages which arrived at the same time but weren't moved by a filter) so the notification wasn't empty, but I don't think the message from the filter target was notified. Or perhaps it's a timing issue. But for POP3 I think the e-mail is stored directly in the target folder without ever hitting the inbox at least if you run it before junk classification.

Marco, you are the reporter here. What is your setup that produces the blank messages? Do you have a filter that moves new messages into a (local) folder which is not part of the account?

Flags: needinfo?(marcoagpinto)

I see this sometimes too:
I have a POP- and a IMAP account. Normally I am in the IMAP account and when a new mail arrives in the POP account the notification is shown. Then I switch to the POP account and the notification arrives a second time. And this second time it is sometime blank. This is my working profile many years old.

Richard: Any filters involved? Only sometimes blank?

I've just tried this in TB 60. Message is moved and no notification is displayed.

Alice: Can you try this again with these STR:
On your account, IMAP or POP, create a filter to move messages where From contains bugzilla-daemon@mozilla.org to a local folder. Then comment on bug 1343157 to create a bugmail. I think you don't need to wait, you can just "Get Messages".

In TB 60, I don't see any notification, hence also no blank one. In TB 68 or trunk, I get the empty notification. Can you please check where that behaviour changed.

Flags: needinfo?(alice0775)

Richard: We really need a reproducible case. Can you make your observation reproducible? I found a reproducible case, but there might be others.

No filter involved, still on the inbox. But different to the screenshot here where is no title, no subject. Only the notification with the icon on the left. This may be a profile issue as it looks different to the screenshot here. And I have this already since a long time (before TB 60).

(In reply to Jorg K (GMT+2) from comment #27)

Richard: Any filters involved? Only sometimes blank?

I've just tried this in TB 60. Message is moved and no notification is
displayed.

Alice: Can you try this again with these STR:
On your account, IMAP or POP, create a filter to move messages where From
contains bugzilla-daemon@mozilla.org to a local folder. Then comment on
bug 1343157 to create a bugmail. I think you don't need to wait, you can
just "Get Messages".

In TB 60, I don't see any notification, hence also no blank one. In TB 68 or
trunk, I get the empty notification. Can you please check where that
behaviour changed.

TB 52 : no notification sound, no notification popup
TB 60 : no notification sound, no notification popup
TB 69.0a1: no notification sound, no notification popup

Flags: needinfo?(alice0775)

Clearly the de-XBL guys broke this :-(

Assignee: mkmelin+mozilla → jorgk
Attachment #9069165 - Attachment is obsolete: true
Flags: needinfo?(marcoagpinto)
Attachment #9069182 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9069182 [details] [diff] [review]
1547202-fix-empty-notification.patch

Review of attachment 9069182 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/content/newmailalert.js
@@ +109,5 @@
>  
>  // If the user initiated the alert, show it right away, otherwise start opening the alert with
>  // the fade effect.
>  function showAlert() {
> +  if (!document.getElementById("folderSummaryInfo").hasMessages()) {

Maybe it would be better to make 'hasMessages' a getter as it was previously (see https://searchfox.org/comm-central/source/suite/mailnews/content/mailWidgets.xml#1860) instead of a function, when it just returns a bool.
Comment on attachment 9069182 [details] [diff] [review]
1547202-fix-empty-notification.patch

Review of attachment 9069182 [details] [diff] [review]:
-----------------------------------------------------------------

Great! 
I'd do it like in the patch. hasMessages isn't really a property, and having it be a function matches with e.g. hasChildNodes()
Attachment #9069182 - Flags: review?(mkmelin+mozilla) → review+

I don't care as long as the annoying empty notifications stop. Interesting by-product: Now we know that messages moved "away" by a filter won't be notified by design.

Keywords: leave-open

(In reply to Jorg K (GMT+2) from comment #25)

Marco, you are the reporter here. What is your setup that produces the blank messages? Do you have a filter that moves new messages into a (local) folder which is not part of the account?

@Jörg

This happens in the 14'' laptop where I have beta + daily and I believe I have the old Telepac account set there which no longer exists and also the test address of Wayne, so, the message it detects as new are in Wayne's test address when I run the daily or beta... but it has been weeks (?) since I last ran the beta/daily... I can't remember.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/28e2abca5938
fix empty notification by retrieving .hasMessages() properly. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Attachment #9069182 - Flags: approval-comm-beta+
Attachment #9068291 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.