Closed Bug 12165 Opened 21 years ago Closed 19 years ago

Get Messages should behave correctly for multiple accounts

Categories

(SeaMonkey :: MailNews: Message Display, defect, P1)

All
Windows NT
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.8

People

(Reporter: nbaca, Assigned: racham)

References

Details

(Whiteboard: [nsbeta3-][nsbeta1+])

Attachments

(8 files)

Get Messages needs to support multiple accounts.
Main Mail Spec http://gooey/client/5.0/specs/mail/messenger/messenger.html.
QA Contact: lchiang → nbaca
Setting QA Contact to nbaca, and Cc: lchiang.
Blocks: 10791
Assignee: phil → alecf
Reassign to alecf
Status: NEW → ASSIGNED
Summary: [FEATURE]Get Messages should prompt for password if account selected → [FEATURE]Get Messages should behave correctly for multiple accounts
Target Milestone: M11
Target Milestone: M11 → M14
This can probably wait until after PR1... Personally I think it's just not worth
the effort right now
*** Bug 23688 has been marked as a duplicate of this bug. ***
M15
Target Milestone: M14 → M15
Target Milestone: M15 → M17
Priority: P3 → P1
at this point this is just a bug, not a feature.
Summary: [FEATURE]Get Messages should behave correctly for multiple accounts → Get Messages should behave correctly for multiple accounts
mass moving to M18 and adding nsbeta3 keyword
Keywords: nsbeta3
Target Milestone: M17 → M18
Adding mail2 nomination keyword for triage effort.
Keywords: mail2
Whiteboard: [nsbeta3-]
Target Milestone: M18 → Future
- per mail triage
It would be nice if this were implemented like in 'trn', the news reader. (The
one where all you ever needed to do was press RETURN.)

Perhaps the NEXT button should cycle through all the unread mail in all the
users accounts, and would get new messages in the next account as it moves along
(requesting the password as necessary.)

sorry for the extra email. Removing mail2 keyword.
Keywords: mail2
over to sspitzer
Assignee: alecf → sspitzer
Status: ASSIGNED → NEW
*** Bug 61918 has been marked as a duplicate of this bug. ***
cc'ing self
marking nsbeta1+. moving to mozilla0.8. reassigning to racham.
Assignee: sspitzer → racham
Whiteboard: [nsbeta3-] → [nsbeta3-][nsbeta1+]
Target Milestone: Future → mozilla0.8
Status: NEW → ASSIGNED
Accepting...
Here are the several things that this fix does..(from spec :
http://www.mozilla.org/mailnews/specs/threepane/GetMail.html, Sections 2 & 3)

Highlights :

* Added menu dropdown to GetMsg button
* Another drop down in FileMenu item (Get New Messages for)
* Get messages for multiple logged in accounts (when Get All New Messages is
selected)
* Accelarators for Getting all Messages

Patch coming up...
Attached patch patch..Splinter Review
mscott, bienvenu, 

please review the patch..thanks.
Bhuvan, you shouldn't use NS_ENSURE_SUCCESS on boolean routines like this one:
PRBool
+nsMsgAccountManagerDataSource::canGetMessages(nsIMsgIncomingServer *aServer)
+{

because NS_ENSURE_SUCCESS(rv, rv) will return rv.
Hey Bhuvan, please don't see me on bugs for a super review. Per the reviewer's
guideline, just send me email with the bug # and a link to the bug. Thanks!

couple comments,
Instead of adding this new method for hasSessionPassword, you could just call
::GetPassword on the incomingServer. This method won't cause us to prompt for
UI. If you get a password back, then you can go ahead and try to get new mail
for it.

I was confused by the need for a canGetNewMessages method? Why do we need a
method that does this? Seems like we defined it to return TRUE for all of our
mail protocols anyway.

Everything else looked good (plus david's comment about NS_ENSURE_SUCCESS)


Bienvenu,
thanks for the comment.  I have modified that to return false, in case of
failure. Will post the updated patch soon.

Scott,
thanks for your comments. 

In the incomingServer case, I can use GetPassword too. The reason I introduced
hasSessionPassword is that we may have to make attribute password as a noscript
attribute for security puposes at some point as it returns password in clear
text now (one has to get through xpconnect though to get this). Are there any
reasons why it is not [noscript] today..? 
 
Regd canGetNewMessages property..today in several places in the js code, we have
been using hardcoded strings like in (server.type != "nntp" && server.type
!="none") and similar ones before we take some action. So, I introduced this
property in the protocolInfo and with that we can simply say if
(protocolType.canGetMessages) without any hardcoded strings..This particular
property (canGetMessages) is decided based on protocol type itself..so I put it
in MsgProtocolInfo.

Please let me know your comments. thanks.
racham: XPConnect security is required to prevent untrusted access to sensitive
code as well as data -- IOW, for lots of reasons, least of all to protect the
password attribute.  You write "we may have to make attribute password as a
noscript attribute for security puposes at some point as it returns password in
clear text now".  Why "may", and who is "we"?  Cc'ing mstoltz.

/be
Posting the new patch updated with the suggestions made.

* Returning PR_FALSE in canGetMessages in ENSURE_SUCCESS cases as suggested by 
David.

* Using the password attribute directly (and removing hasSessionPassword) to see 
if the mail session has password for the server queried as suggested by mScott.

I asked about password attribute status only to know if there is an intended 
purpose to have it defined that way....here comes one of the uses. I am now 
using it in may patch to query to see if the password for a given session is  
avaiable.

please review the updated patch..thanks.
Attached patch updated patchSplinter Review
str.AssignWithConversion("true");
I think AssignWithConversion is taboo. NS_LITERAL_STRING("true")

<span class="whitespace" style="display: collapsed">
+                                 <menuitem value="&getAllNewMsgCmd.label;" 
+                                          key="key_getAllNewMessages"
can you align key w/ value?

For the xul files, can you try and limit line width (80-2 cols is ideal, beyond 
115 is too long even for overwide editor/browser views).

it looks like you used tabs instead of 4 spaces:
RCS file: /cvsroot/mozilla/mailnews/imap/src/nsImapService.cpp,v
RCS file: /cvsroot/mozilla/mailnews/local/src/nsPop3Service.cpp,v
This file seems to have 7space indentation :!!
RCS file: /cvsroot/mozilla/mailnews/news/src/nsNntpService.cpp,v

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/src/nsMsgAcco
untManagerDS.cpp
alecf in 1.42 selected 4 4 as the modeline and checked in indentations of 2.
</span>
Will post a new patch. thanks.
this:
+        var allServers = accountManager.allServers;
+     
+        for (var i=0;i<accountManager.allServers.Count();i++) 
+        {
+            var currentServer =
accountManager.allServers.GetElementAt(i).QueryInterface(Components.interfaces.nsIMsgIncomingServer);

is absurdly inefficient. the allServers call takes an O(n) time to construct,
which means you're taking this O(n) loop and turning into an O(n^2) operation.
Strangely, you do cache "allServers" right above. why not use that?


also, you're defining two identical places where the menuitems or toolbar items
do the exact same thing (i.e. the oncommand= is the same) - in this case you
should be using a command node, and putting oncommand there, then observing the
command node with observes="<commandid>"
do you really want to do both these in messagewindow.js:

+                                MsgGetMessage();
+                                MsgGetMessagesForAllAuthenticatedAccounts();

Isn't the second one sufficient?
David, 

You are right. The current account is already authenticated..thanks. Will post a
new patch for messagewindow.js
r=bienvenu
bhuvan,

there is already an implementation of "GetServer(uri)" in msgMail3PaneWindow.js

instead of duplicating it in messagewindow.js, let's move it to a shared .js file.

can you do that, and attach the entire patch (in one diff) to this bug report.
Looks like commandglue.js is the right place. New & total patch coming up soon...
Attached patch complete patchSplinter Review
looks good.  

for nsMoveMailService,  you should have

*aCanGetMessages = PR_TRUE;

since you are in the code anyways, can you fix
nsMsgAccountManagerDataSource::GetTarget() to use NS_LITERAL_STRING() instead of
AssignWithConversion?

your patch does this, but the rest of the code doesn't.  it should be easy and
low risk,  and it is more efficent.

do that, and fix nsMoveMailService, and sr=sspitzer
this got checked in according to the Bonsai checkin logs.  So, marking fixed for
racham.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Ack, I think bug 68047 is a regression of this checkin! :(

That bug is simply terrible, it breaks the GetMsg button's ui totally. Very visible.
Build 2001-02-19-08: NT4, Linux RH 6.2, Mac 9.04
Verified Fixed. All items described by Bhuvan on 2/2/2001 are working.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.