Closed
Bug 12165
Opened 25 years ago
Closed 24 years ago
Get Messages should behave correctly for multiple accounts
Categories
(SeaMonkey :: MailNews: Message Display, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.8
People
(Reporter: nbaca, Assigned: racham)
References
Details
(Whiteboard: [nsbeta3-][nsbeta1+])
Attachments
(8 files)
19.23 KB,
patch
|
Details | Diff | Splinter Review | |
17.71 KB,
patch
|
Details | Diff | Splinter Review | |
17.29 KB,
patch
|
Details | Diff | Splinter Review | |
5.11 KB,
patch
|
Details | Diff | Splinter Review | |
20.23 KB,
patch
|
Details | Diff | Splinter Review | |
1.98 KB,
patch
|
Details | Diff | Splinter Review | |
1.93 KB,
patch
|
Details | Diff | Splinter Review | |
23.39 KB,
patch
|
Details | Diff | Splinter Review |
Get Messages needs to support multiple accounts. Main Mail Spec http://gooey/client/5.0/specs/mail/messenger/messenger.html.
Reporter | ||
Updated•25 years ago
|
QA Contact: lchiang → nbaca
Reporter | ||
Comment 1•25 years ago
|
||
Setting QA Contact to nbaca, and Cc: lchiang.
Updated•25 years ago
|
Assignee: phil → alecf
Comment 2•25 years ago
|
||
Reassign to alecf
Updated•25 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•25 years ago
|
Target Milestone: M11 → M14
Comment 3•25 years ago
|
||
This can probably wait until after PR1... Personally I think it's just not worth the effort right now
Updated•25 years ago
|
Target Milestone: M15 → M17
Updated•25 years ago
|
Priority: P3 → P1
Comment 6•24 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•24 years ago
|
||
mass moving to M18 and adding nsbeta3 keyword
Keywords: nsbeta3
Target Milestone: M17 → M18
Comment 10•24 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 13•24 years ago
|
||
*** Bug 61918 has been marked as a duplicate of this bug. ***
Comment 14•24 years ago
|
||
cc'ing self
Keywords: nsbeta1
Comment 15•24 years ago
|
||
marking nsbeta1+. moving to mozilla0.8. reassigning to racham.
Assignee: sspitzer → racham
Whiteboard: [nsbeta3-] → [nsbeta3-][nsbeta1+]
Target Milestone: Future → mozilla0.8
Assignee | ||
Comment 16•24 years ago
|
||
Accepting...
Assignee | ||
Comment 17•24 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•24 years ago
|
||
Assignee | ||
Comment 19•24 years ago
|
||
mscott, bienvenu, please review the patch..thanks.
Comment 20•24 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•24 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•24 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.
Comment 23•24 years ago
|
||
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•24 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•24 years ago
|
||
Comment 26•24 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•24 years ago
|
||
Will post a new patch. thanks.
Assignee | ||
Comment 28•24 years ago
|
||
Assignee | ||
Comment 29•24 years ago
|
||
Comment 30•24 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•24 years ago
|
||
Assignee | ||
Comment 32•24 years ago
|
||
Comment 33•24 years ago
|
||
do you really want to do both these in messagewindow.js: + MsgGetMessage(); + MsgGetMessagesForAllAuthenticatedAccounts(); Isn't the second one sufficient?
Assignee | ||
Comment 34•24 years ago
|
||
David, You are right. The current account is already authenticated..thanks. Will post a new patch for messagewindow.js
Assignee | ||
Comment 35•24 years ago
|
||
Comment 36•24 years ago
|
||
r=bienvenu
Comment 37•24 years ago
|
||
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•24 years ago
|
||
Looks like commandglue.js is the right place. New & total patch coming up soon...
Assignee | ||
Comment 39•24 years ago
|
||
Comment 40•24 years ago
|
||
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•24 years ago
|
||
this got checked in according to the Bonsai checkin logs. So, marking fixed for racham.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 42•24 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•24 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
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•