Integrate new IM notifications in nsMessengerOSXIntegration.mm

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Thunderbird 16.0
x86
Mac OS X

Thunderbird Tracking Flags

(thunderbird15 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 636304 [details] [diff] [review]
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
Blocks: 742746
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)
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.
(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 ;).
Created attachment 636368 [details] [diff] [review]
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.
Assignee: nobody → florian
Attachment #636304 - Attachment is obsolete: true
Created attachment 637106 [details] [diff] [review]
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.
Attachment #636368 - Attachment is obsolete: true
Attachment #637106 - Flags: ui-review?(bwinton)
Attachment #637106 - Flags: review?(dbienvenu)
(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

5 years ago
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;
(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 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.
Attachment #637106 - Flags: ui-review?(bwinton) → ui-review+

Comment 10

5 years ago
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.
Attachment #637106 - Flags: review?(dbienvenu) → review+
(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 :).
https://hg.mozilla.org/comm-central/rev/07edf83b2991
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Attachment #637106 - Flags: approval-comm-aurora?
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.
See Also: → bug 715799
(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...
Attachment #637106 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/b824c2c6054c
status-thunderbird15: --- → fixed
You need to log in before you can comment on or make changes to this bug.