Last Comment Bug 337052 - Reactivate WarpCenter biff functionality in nsMessengerOS2Integration
: Reactivate WarpCenter biff functionality in nsMessengerOS2Integration
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 OS/2
: -- minor (vote)
: ---
Assigned To: Peter Weilbacher
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-07 13:35 PDT by Peter Weilbacher
Modified: 2008-07-31 01:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix it and clean up (8.20 KB, patch)
2006-05-07 13:47 PDT, Peter Weilbacher
no flags Details | Diff | Splinter Review
Updated per Neil's comments (7.70 KB, patch)
2006-05-16 23:50 PDT, Peter Weilbacher
mozilla: review+
mozilla: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Peter Weilbacher 2006-05-07 13:35:30 PDT
Ever since the checkin(s) from bug 226270 the WarpCenter biff functionality on OS/2 was broken. I have a patch that reactivates it.
Comment 1 Peter Weilbacher 2006-05-07 13:47:39 PDT
Created attachment 221234 [details] [diff] [review]
Fix it and clean up

The main change is in nsMessengerOS2Integration::OnItemIntPropertyChanged where one needs to wrap the tests on nsIMsgFolder::nsMsgBiffState_ within a check for (aProperty == mBiffStateAtom). When the method gets called with (aProperty == mTotalUnreadMessagesAtom) the number actually means a message count and not just the message state.

The question I still have is if I have to test for the server type or if nsMessengerOS2Integration::OnItemIntPropertyChanged only gets called for POP3 and IMAP servers anyway.

The cleanup in this patch is largely cosmetic, as usual the file had inconsistent leading blanks. I also renamed pUnreadCount to pUnreadState as we also set state changes (to keep track of the unread number of messages is unnecessary for the WarpCenter functionality).
Comment 2 Peter Weilbacher 2006-05-07 13:49:06 PDT
Neil, perhaps you can tell me if I need to test for the server type...
Comment 3 neil@parkwaycc.co.uk 2006-05-07 14:10:56 PDT
Sorry for breaking biff on OS/2; I did't realise that the other platforms happened not to break because they had the atom test.

(In reply to comment #2)
>Neil, perhaps you can tell me if I need to test for the server type...
I wouldn't; the other platforms don't test, plus if we should decide to support a new server type (webmail perhaps) then we would want biff to work there too.
Comment 4 neil@parkwaycc.co.uk 2006-05-07 14:14:17 PDT
Comment on attachment 221234 [details] [diff] [review]
Fix it and clean up

> NS_IMPL_ADDREF(nsMessengerOS2Integration)
> NS_IMPL_RELEASE(nsMessengerOS2Integration)
> 
> NS_INTERFACE_MAP_BEGIN(nsMessengerOS2Integration)
>-   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMessengerOSIntegration)
>-   NS_INTERFACE_MAP_ENTRY(nsIMessengerOSIntegration)
>-   NS_INTERFACE_MAP_ENTRY(nsIFolderListener)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMessengerOSIntegration)
>+  NS_INTERFACE_MAP_ENTRY(nsIMessengerOSIntegration)
>+  NS_INTERFACE_MAP_ENTRY(nsIFolderListener)
> NS_INTERFACE_MAP_END
Or consider using NS_IMPL_ISUPPORTS2(nsMessengerOS2Integration, nsIMessengerOSIntegration, nsIFolderListener)
Comment 5 Peter Weilbacher 2006-05-16 23:50:53 PDT
Created attachment 222316 [details] [diff] [review]
Updated per Neil's comments

Neil, thanks for your hints, I take them both into account in this new version of the patch. Btw, the most suprising thing is that it took OS/2 users so long to complain about the bug and someone to find out how the status is communicated to the system to get me something concrete to look for. I didn't even know about this class three weeks ago...

Mike, if you are happy with this, I think we can use the patch on trunk and both branches.
Comment 6 Mike Kaply [:mkaply] 2006-05-17 07:19:45 PDT
I thought the WarpCenter could actually show a count of messages when you hover over it or something, and that's why I was setting the number of unread and not just the state?
Comment 7 Peter Weilbacher 2006-05-17 07:45:35 PDT
I hope it doesn't because that would complicate matters a lot. (I do not have a WarpCenter around at the moment for testing but nobody mentioned that before.) This
http://groups.google.de/group/netscape.public.mozilla.os2/msg/d1ccc45ab022d8ab?dmode=source&hl=de
is an old message that you obviously posted when you had originally implemented the feature. From that it seems that the actual number of new messages was never shown.

The only other program that I know that makes use of this shared memory object (HBMozillaMail for the XCenter from http://www.os2usr.org/xcenter/) also only shows new mail when it is set to 1 and shows "no mail" otherwise. At least that's what I see in its sources.
Comment 8 Mike Kaply [:mkaply] 2006-05-17 08:23:47 PDT
Comment on attachment 222316 [details] [diff] [review]
Updated per Neil's comments

cool

these are platform specific, So I'll approve them
Comment 9 Daniel Veditz [:dveditz] 2006-05-19 11:34:28 PDT
Comment on attachment 222316 [details] [diff] [review]
Updated per Neil's comments

The tree is closed for 1.8.0.4 and this didn't make it... moving approval to 1.8.0.5.
Comment 10 Mike Kaply [:mkaply] 2006-05-22 18:33:05 PDT
fix checked in.
Comment 11 Daniel Veditz [:dveditz] 2006-06-12 11:58:54 PDT
Comment on attachment 222316 [details] [diff] [review]
Updated per Neil's comments

approved for 1.8.0 branch, a=dveditz for drivers

Note You need to log in before you can comment on or make changes to this bug.