Using 2001050704 Build on WinNT 4.0 choosing the File | Offline | Offline settings menu item doesn't bring up the 'Account settings-Offline & storage' dialog. Nothing happens.

assign to myself

Bhuvan, Seth, could I get your R&SR?

This patch looks good to me (if my review is OK instead of bhuvan's). r=hwaara

Sure, thanks for looking into it, hwaara

no go on this patch. I don't think it will work in the stand alone message window. DoesAccountHaveOfflineFunc() will not be defined. (make sure to test the stand alone message window.) DoesAccountHaveOfflineFunc() has problems. see below +function DoesAccountHaveOfflineFunc() +{ + + var tree = GetFolderTree(); + var folderList = tree.selectedItems; + if(folderList.length >= 1) + { + for(var i = 0; i<folderList.length; i++) + { + var folderNode = folderList[i]; + if(folderNode.getAttribute("ServerType") == "imap" || + folderNode.getAttribute("ServerType") == "pop3" || + folderNode.getAttribute("ServerType") == "pop" || + folderNode.getAttribute("ServerType") == "nntp") + return true; first, there is no server type of pop. second, I'm against hard coding types like this in our js. bhuvan and I have been cleaning this up as we can, and been making sure not to add new code that does it. here's what you should do: if num selected folders != 1, return false. we currently don't support multiple selection in the folder pane, but once we do, you should be disabled in that case. from the "ServerType" for the selected folder, get the appropriate nsIMsgProtocolInfo service. see am-server.js for some sample code. then, do something like: return protocolinfo.supportsOffline; Now, you have to extend the nsIMsgProtocolInfo and add a new boolean attribute, supportsOffline. (feel free to choose a better name). I'm assuming that all imap, pop3 and nntp servers will support offline, and all "none" and "movemail" servers will not. if it was a server by server thing, this new attribute should be in nsIMsgIncomingServer and you'd be getting that for the selected folder, instead of the nsIMsgProtocolInfo. I'd make the default implementation for GetSupportsOffline() set the out param to PR_FALSE, and override it in nsNntpService, nsImapService and nsPop3Service to return PR_TRUE. bhuvan is the master of doing this. please see him (or me) for help. and have

> and have whoops, not finished yet. perhaps nsIMsgProtocolInfo.offlineLevelSupported could be an int attribute. forgive my offline ignorance. (bienvenu, can you chime in?) 0 == no support 1 == normal (imap, pop3 level) 2 == extended (retention / download for nntp) 3 == ? in DoesAccountHaveOfflineFunc(), you'd check that the offlineSupportLevel != 0. then you, could fix am-offline.js. instead of checking the type against "nntp" you could check if (offlineSupportLevel >= 2) then blindly pass the server type down into initDownloadSettings() and initRetentionSettings(). that would allow you to fix the hard coding in am-offline.js. the current code won't work well if we ever added retention or download settings to an existing or new server type. please work with bhuvan to clean this up properly.

well, first of all, pop doesn't have any offline settings (it has a disk space setting, but no offline settings). Doing it the right way is a good idea because we very well might get asked to do retention settings for imap, for example.

Tested, DoesAccountHaveOfflineFunc() works for standalone message window. Seth, David pointed out good approach which requires the touch of backend.

I was agreeing with Seth's approach.

you should fix this in one fell swoop. updating summary to cover the issues. plan of attach: add nsIMsgProtocolInfo.offlineSupportLevel, implement it and use it in the 3 pane and am-offline.js

msgAccountCentral.js also has plenty of examples (onInit() & ArrangeAccountCentralItems()) the way you can use the attributes to determine the server/protocol capabilities instead of hardcoded strings. This feature seems like a protocol based one. You can the get 'id' attribute from your folderList for each and then the use GetServer to get the server. From there get the protocol info and query for the capability. A boolean will suffice here. If the extended feature capability (as Seth suggested having 0, 1, 2 etc) is to be determined then you may need to int to represent various states. The changes required in the backend are pretty simple. You can query on any of the attributes (used in msgAccountCentral.js) in lxr to find more about the things done on backend. Please let me know if you have any questions. bhuvan

On second thoughts..I think we need to add the offlineSupportLevel attribute at server level not the protocol level. We need to have flexibility to allow a given server to turn on/off it's offline services. ISPs might need this kind of behavior. Some might want to support and some may not. I needed this feature of determining the offline support for AccountCentral. I am going to go ahead and add the support needed on the backend. If I am left with any cycles, I will come back and revisit that patch posted in here. bhuvan

sounds good. do you have a bug for that work? this bug should be blocked by that work. diane [you, or someone external] shouldn't bother to work on this until you're done with adding the new attribute to the nsIMsgIncomingServer interface.

Sounds great.

Adding the dependency (AccountCentral bug 76388).

oops...correcting the dependency.

nominating

*** Bug 82490 has been marked as a duplicate of this bug. ***

if we fix #82492 properly, we can have the supportsOffline be a boolean attribute of the nsIMsgFolder (which will get its value by checking its server's offline level, and if <= 0, return false). we can get the current nsIMsgFolder from the current selection in the folderpane.

Since bug 82492 was marked as a dupe. The correct bug, for Seth's comments on 2001-05-23 19:44 , is bug 79239

Diane, Don't you still need to take care of messageWindow case ? It simply returns true always today. http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/messageWindow.js#529 && Also, move function DoesAccountHaveOfflineSupport() into mailWindowOverlay.js so that both js files (mail3PaneWindowCommands.js & messageWindow.js) have access to the function. Please update the patch with the above changes. Will be quick on reviews on the upcoming patch (this & other bugs too). thanks, bhuvan.

r=bhuvan. window.parent.MsgAccountManager()is OK for now. But it got to change when we can actually open the AccountManager window and select offline settings item of a particular server. I have a bug on that. I will cc you one that one. bhuvan

PDT+ per 6/12 mtg.

Request SR again.

sr=bienvenu, if you fix this indentation: +function DoesAccountHaveOfflineSupport() +{ + var selectedFolders = GetSelectedMsgFolders(); + + if (selectedFolders && selectedFolders.length > 0) var should be at the same level of indentation as if, I would think.

sr=sspitzer, but fix the indentation problem as bienvenu pointed out. note, once I land my fix for http://bugzilla.mozilla.org/show_bug.cgi?id=79239 we'll be able to fix this code to use folder.supportsOffline

indentation is fixed. Request for apporval.

I bet you already knew that you need to email drivers@mozilla.org for that..

a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)

Checking in commandglue.js; /cvsroot/mozilla/mailnews/base/resources/content/commandglue.js,v <-- commandg lue.js new revision: 1.172; previous revision: 1.171 done Checking in mail3PaneWindowCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/mail3PaneWindowCommands.js,v < -- mail3PaneWindowCommands.js new revision: 1.52; previous revision: 1.51 done Checking in mailWindowOverlay.js; /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v <-- ma ilWindowOverlay.js new revision: 1.73; previous revision: 1.72 done Checking in messageWindow.js; /cvsroot/mozilla/mailnews/base/resources/content/messageWindow.js,v <-- messag eWindow.js new revision: 1.33; previous revision: 1.32 done

note, when you checked in you didn't specify a check in comment. also, when I fix #79239, I'll fix your implementation of DoesAccountHaveOfflineSupport() to use nsIMsgFolder.supportsOffline

I've spun off using .supportsOffline (and another issue) to bug #86187

one last comment, until #79239 is fixed, "Offline Settings" will be enabled for pop. as soon as I land my fix for #79239, that will be fixed. (my fix includes fixing the offline support level for pop accounts.)

I lied, one more problem. the patch that got checked in cause js exceptions in the stand alone msg window. ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "MailAreaHasFocus is not defined" {file: "chr ome://messenger/content/messageWindow.js" line: 531}]' when calling method: [nsI Controller::isCommandEnabled]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_E RROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverla y.js :: goUpdateCommand :: line 79" data: yes] ************************************************************ An error occurred updating the cmd_settingsOffline command ************************************************************ * Call to xpconnect wrapped JSObject produced this error: * [Exception... "'[JavaScript Error: "MailAreaHasFocus is not defined" {file: "chr ome://messenger/content/messageWindow.js" line: 531}]' when calling method: [nsI Controller::isCommandEnabled]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_E RROR_WITH_DETAILS)" location: "JS frame :: chrome://global/content/globalOverla y.js :: goUpdateCommand :: line 79" data: yes] ************************************************************ I'll fix that with my patch to #86187

more fallout, what got checked in causes js assertions on both the 3 pane and the stand alone msg window. I'll attach the fix.

Oops, made an error while checking in. Thanks so much for taking care of it. Have you checked the fix.

fix checked in.

actually, you checked in another js error. + return DoesAccountHaveOfflineSupport()); should have been + return DoesAccountHaveOfflineSupport(); I've fixed it. also, please specify comments when you check in. all your checkins today did not have any comments. see http://bonsai.mozilla.org/cvsquery.cgi? module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=day&who=dianesun% 25netscape.com

You actually changed the function name from DoesAccountHaveOfflineSupport() to IsOfflineSettingsEnabled().

Using commercial builds: 2001-06-19-11-trunk/ -win nt 4.0 2001-06-19-08-trunk/ mac os 9.0.4 2001-06-18-08-trunk/ linux 2.2, red hat 7.0 Verified the following: -Selecting File|Offline|Offline Settings in Messenger, now brings up the Account setting panel for that particular mail account [for imap accounts] -Selecting File|Offline|Offline Settings in a seperate mesg window, now brings up the Account setting panel for that particular mail account [for imap accounts] -Selecting File|Offline|Offline Settings for a newsgroup in Messenger, now brings up the Account setting panel for that particular news account -User cannot select File|Offline|Offline Settings for a POP account in Messenger or in a seperate POP mesg window -User cannot select File|Offline|Offline Settings for Local Folders in Messenger or in a seperate Local Folders mesg window Note there is a minor bug in that I believe when you select Offline Settings it should take you to "Offline and Disk Space" panel rather than to the account panel. The bug number is bug 86768. A question: Is this fix implemented for newsgroups? If the answer is yes, i noticed when you bring up a seperate news message window up and select offline settings, it DOESN'T take you to News account but to the first default mail account. Works fine if you select the newsgroup server, newsgroup account, etc.. I will not verify the fix, till I get an answer (either file a seperate bug or the fix wasn't intended for newsgroups)

> A question: Is this fix implemented for newsgroups? If the answer is > yes, i noticed when you bring up a seperate news message window > up and select offline settings, it DOESN'T take you to News account > but to the first default mail account. Works fine if you select > the newsgroup server, newsgroup account, etc.. If you mean to say that: when reading a news message in the stand alone message window, "File | Offline | Offline Settings" opens up the account manager to the default account. then yes, that's a bug. my guess as to why that is happening is that we are using the message url (which for a news message is news://host/message-id) instead of the folder url from the gDBView. can you log a new bug on dianesun and cc me? thanks.

That's Exactly what I mean Seth. Ok. I'll file a new bug on that and i'll finish verifying this bug.

Commercial builds 2001-06-21-09-trunk/ WinNT 4.0 2001-06-21-11-trunk/ Linux 2.2, red hat 7.0 2001-06-21-08-trunk/ MAC 9.0.4 Reverified the following: -Selecting File|Offline|Offline Settings in Messenger, now brings up the Account setting panel for that particular mail account [for imap or webmail accounts] -Selecting File|Offline|Offline Settings for a newsgroup in Messenger, now brings up the Account setting panel for that particular news account -User cannot select File|Offline|Offline Settings for a POP account in Messenger or in a seperate POP mesg window -User cannot select File|Offline|Offline Settings for Local Folders in Messenger or in a seperate Local Folders mesg window I was mistaken about it working correctly if you bring up a seperate mail message window. It doesn't open to the correct account. The same thing happens in a seperate news message window. That bug about Offline Settings not opening to correct account if you have a mail or news message in a seperate window is bug 87239 Marking as verified.