767987 - Integrate new IM notifications in nsMessengerOSXIntegration.mm

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Florian Quèze [:florian]]

Attachment: WIP 1

Attaching a first WIP that bounces the dock icon when new IMs arrive.

Other parts that need to be handled:
- popup (Growl) notifications
- unread count in the dock icon badge

Comment 1

[Ludovic Hirlimann [:Usul]]

Comment on attachment 636304 [details] [diff] [review]
WIP 1

With this is there a way to have growl without the dock icon ? (for email I don't mind for chats I do)

Comment 2

[:Irving Reid (No longer working on Firefox)]

For Growl, you can use the alerts service directly from scripts: https://developer.mozilla.org/en/XUL_School/User_Notifications_and_Alerts#The_Alerts_Service; I'm in the process of moving all this code out of the system-specific C++ / Objective-C platform integration modules.

For badging the doc icon with unread counts, I'd prefer if it could be added to the new code in bug 715799, we're trying to get that landed in the next few weeks.

Comment 3

[Florian Quèze [:florian]]

(In reply to Irving Reid (:irving) from comment #2)

> For badging the doc icon with unread counts, I'd prefer if it could be added
> to the new code in bug 715799, we're trying to get that landed in the next
> few weeks.

I'm targeting Tb15 here ;).

Comment 4

[Florian Quèze [:florian]]

Attachment: WIP 2

- This handles the unread count on the dock icon. Handling this in this compiled file makes sense.

- This also handles popup (growl) notifications, but I'm not fully happy with them yet.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v3

This is attachment 636368 [details] [diff] [review] without the growl notifications that I removed because I'm not satisfied of how they work:
- they showed the HTML code of messages instead of the plain text version
- they happened even if the conversation was selected and focused, so it was way too noisy.
I don't see good ways to fix this in C++, so if I do growl notifications again, I think I'll handle them from a JS component, or even directly from the UI code.

The attached patch adds support for:
- badging the dock icon with unread messages (note: the messages are counted as unread only if the conversation or the chat tab isn't selected.)
- bouncing the dock icon when a new message arrives and the Thunderbird window isn't focused.

Comment 6

[Florian Quèze [:florian]]

(In reply to Florian Quèze from comment #5)

> - bouncing the dock icon when a new message arrives and the Thunderbird
> window isn't focused.

This will only work if the imConversations.js hunk of the patch from bug 755718 is already applied.

Comment 7

[David :Bienvenu]

Comment on attachment 637106 [details] [diff] [review]
Patch v3

I haven't run with this yet, but it looks reasonable, except for the following nit:

uint means unreadint, not unsigned int, right? I'd change the name, because uint is much more associated with unsigned int, and it's confusing, because it's really signed.

+      let uint = Components.classes["@mozilla.org/supports-PRInt32;1"]
+                           .createInstance(Ci.nsISupportsPRInt32);
+      uint.data = unreadCount;

Comment 8

[Florian Quèze [:florian]]

(In reply to David :Bienvenu from comment #7)
> Comment on attachment 637106 [details] [diff] [review]
> Patch v3
> 
> I haven't run with this yet, but it looks reasonable, except for the
> following nit:
> 
> uint means unreadint, not unsigned int, right?

It meant unsigned int, because I used an unsigned value (it makes no sense to have a negative number of unread messages), but then I changed to a signed int when I started using the value in nsMessengerOSXIntegration.mm for consistency with the other mUnreadTotal and mNewTotal values. I didn't want to have compiler warnings about mixing signed and unsigned :).

I'll change the name in the next iteration of the patch.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 637106 [details] [diff] [review]
Patch v3

Seems okay.  ui-r=me.  Although I didn't see an email bounce when I had an outstanding IM bounce.  Can you double-check that that works for you before checkin?

Thanks,
Blake.

Comment 10

[David :Bienvenu]

Comment on attachment 637106 [details] [diff] [review]
Patch v3

I rather agree with Irving about not propagating the biff pref name. And you're setting yourself up for migration pain if you plan on using a different pref in the future. Is there going to be UI for this pref? If so, I'd really suggest just going with the name you ultimately intend to use.

Comment 11

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)
> I didn't see an email bounce when I had an
> outstanding IM bounce.  Can you double-check that that works for you before
> checkin?

This is because the dock icon bounces for new emails only in cases where a growl notification is displayed, see: http://hg.mozilla.org/comm-central/annotate/911e68201428/mailnews/base/src/nsMessengerOSXIntegration.mm#l377

I think a growl notification is displayed only if there was no new messages before, or something like that.


(In reply to David :Bienvenu from comment #10)

> I rather agree with Irving about not propagating the biff pref name. 

I think this comment was meant for bug 755718. I'm changing the pref name to "mail.chat.play_notification_sound" before the check-in.

> you're setting yourself up for migration pain if you plan on using a
> different pref in the future.

I don't have clear future plans for IM sounds yet, but I expect users will want to have some UI to configure different sounds for IM and email.

> Is there going to be UI for this pref?

Not in the near future (ie Tb15-16), but in the future... maybe :).

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/07edf83b2991

Comment 13

[:Irving Reid (No longer working on Firefox)]

Does this need observerService->removeObserver() calls to balance out the AddObserver calls?

+         observerService->AddObserver(this, kNewChatMessageTopic, false);
+         observerService->AddObserver(this, kUnreadImCountChangedTopic, false);

This module also doesn't de-register its folder listener, which is old code:

+    return mailSession->AddFolderListener(this, nsIFolderListener::boolPropertyChanged | nsIFolderListener::intPropertyChanged);

This code will eventually move to the new JS Notification Service in bug 715799, but if these registrations could cause meaningful leaks we might want to fix them.

Comment 14

[Florian Quèze [:florian]]

(In reply to Irving Reid (:irving) from comment #13)
> Does this need observerService->removeObserver() calls to balance out the
> AddObserver calls?

I wondered the same thing, and I think the answer is no because:
- as you pointed out, the existing code in this file doesn't remove the folder listener, so it was probably OK to not clean-up.
- the observers would only be removed at shutdown when destroying the notification service, and the observers service is probably destroyed more or less at the same time; dropping references to all observers.

Would be nice if someone could confirm though, as this is mostly a guess...

Comment 15

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/b824c2c6054c