Get Messages should behave correctly for multiple accounts

VERIFIED FIXED in mozilla0.8

Status

P1
normal
VERIFIED FIXED
19 years ago
14 years ago

People

(Reporter: nbaca, Assigned: racham)

Tracking

Trunk
mozilla0.8
All
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(8 attachments)

(Reporter)

Description

19 years ago
Get Messages needs to support multiple accounts.
Main Mail Spec http://gooey/client/5.0/specs/mail/messenger/messenger.html.
(Reporter)

Updated

19 years ago
QA Contact: lchiang → nbaca
(Reporter)

Comment 1

19 years ago
Setting QA Contact to nbaca, and Cc: lchiang.

Updated

19 years ago
Blocks: 10791

Updated

19 years ago
Assignee: phil → alecf

Comment 2

19 years ago
Reassign to alecf

Updated

19 years ago
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

Updated

19 years ago
Target Milestone: M11 → M14

Comment 3

19 years ago
This can probably wait until after PR1... Personally I think it's just not worth
the effort right now

Comment 4

19 years ago
*** Bug 23688 has been marked as a duplicate of this bug. ***

Comment 5

19 years ago
M15
Target Milestone: M14 → M15

Updated

19 years ago
Target Milestone: M15 → M17

Updated

19 years ago
Priority: P3 → P1

Comment 6

19 years ago
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

Comment 7

18 years ago
mass moving to M18 and adding nsbeta3 keyword
Keywords: nsbeta3
Target Milestone: M17 → M18

Comment 8

18 years ago
Adding mail2 nomination keyword for triage effort.
Keywords: mail2

Updated

18 years ago
Whiteboard: [nsbeta3-]
Target Milestone: M18 → Future

Comment 9

18 years ago
- per mail triage

Comment 10

18 years ago
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.)

Comment 11

18 years ago
sorry for the extra email. Removing mail2 keyword.
Keywords: mail2

Comment 12

18 years ago
over to sspitzer
Assignee: alecf → sspitzer
Status: ASSIGNED → NEW

Comment 13

18 years ago
*** Bug 61918 has been marked as a duplicate of this bug. ***

Comment 14

18 years ago
cc'ing self

Comment 15

18 years ago
marking nsbeta1+. moving to mozilla0.8. reassigning to racham.
Assignee: sspitzer → racham
Whiteboard: [nsbeta3-] → [nsbeta3-][nsbeta1+]
Target Milestone: Future → mozilla0.8
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 16

18 years ago
Accepting...
(Assignee)

Comment 17

18 years ago
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...
(Assignee)

Comment 18

18 years ago
Created attachment 24268 [details] [diff] [review]
patch..
(Assignee)

Comment 19

18 years ago
mscott, bienvenu, 

please review the patch..thanks.

Comment 20

18 years ago
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.

Comment 21

18 years ago
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)


(Assignee)

Comment 22

18 years ago
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
(Assignee)

Comment 24

18 years ago
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.
(Assignee)

Comment 25

18 years ago
Created attachment 24475 [details] [diff] [review]
updated patch

Comment 26

18 years ago
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>
(Assignee)

Comment 27

18 years ago
Will post a new patch. thanks.
(Assignee)

Comment 28

18 years ago
Created attachment 24516 [details] [diff] [review]
updated patch with timeless comments
(Assignee)

Comment 29

18 years ago
Created attachment 24518 [details] [diff] [review]
mailwindowoverlay.xul diffs (slipped indent in one place in the last patch)

Comment 30

18 years ago
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>"
(Assignee)

Comment 31

18 years ago
Created attachment 24587 [details] [diff] [review]
updated patch with Alec's comments
(Assignee)

Comment 32

18 years ago
Created attachment 24617 [details] [diff] [review]
messageWindow also needs to know about the observer..patch for messagewindow.js

Comment 33

18 years ago
do you really want to do both these in messagewindow.js:

+                                MsgGetMessage();
+                                MsgGetMessagesForAllAuthenticatedAccounts();

Isn't the second one sufficient?
(Assignee)

Comment 34

18 years ago
David, 

You are right. The current account is already authenticated..thanks. Will post a
new patch for messagewindow.js
(Assignee)

Comment 35

18 years ago
Created attachment 24627 [details] [diff] [review]
update to messagewindow.js patch

Comment 36

18 years ago
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.
(Assignee)

Comment 38

18 years ago
Looks like commandglue.js is the right place. New & total patch coming up soon...
(Assignee)

Comment 39

18 years ago
Created attachment 24636 [details] [diff] [review]
complete patch
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

Comment 41

18 years ago
this got checked in according to the Bonsai checkin logs.  So, marking fixed for
racham.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 42

18 years ago
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.
(Reporter)

Comment 43

18 years ago
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.