79267 - Offline: File|Offline|Offline Settings doesn't work

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect, P1)

Description

[grylchan]

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.

Comment 1

[Diane Sun]

assign to  myself

Comment 2

[Diane Sun]

Attachment: Bug fix

Comment 3

[Diane Sun]

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

Comment 4

[Håkan Waara]

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

Comment 5

[Diane Sun]

Sure, thanks for looking into it, hwaara

Comment 6

[(not reading, please use [e-mail] instead)]

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

Comment 7

[(not reading, please use [e-mail] instead)]

> 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.

Comment 8

[David :Bienvenu]

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.

Comment 9

[Diane Sun]

Tested, DoesAccountHaveOfflineFunc() works for standalone message window.

Seth, David pointed out good approach which requires the touch of backend.

Comment 10

[David :Bienvenu]

I was agreeing with Seth's approach.

Comment 11

[(not reading, please use [e-mail] instead)]

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

Comment 12

[racham]

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

Comment 13

[racham]

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

Comment 14

[(not reading, please use [e-mail] instead)]

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. 

Comment 15

[Diane Sun]

Sounds great.

Comment 16

[racham]

Adding the dependency (AccountCentral bug 76388).

Comment 17

[racham]

oops...correcting the dependency.

Comment 18

[scottputterman]

nominating

Comment 19

[grylchan]

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

Comment 20

[(not reading, please use [e-mail] instead)]

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.

Comment 21

[grylchan]

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

Comment 22

[Diane Sun]

Attachment: New fix with offlineSupportLevel

Comment 23

[racham]

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.

Comment 24

[Diane Sun]

Attachment: Updated fix with Bhuvan's suggestion

Comment 25

[racham]

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

Comment 26

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 27

[Diane Sun]

Request  SR again.

Comment 28

[David :Bienvenu]

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.

Comment 29

[(not reading, please use [e-mail] instead)]

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

Comment 30

[Diane Sun]

indentation is fixed. Request for apporval. 

Comment 31

[Håkan Waara]

I bet you already knew that you need to email [e-mail] for that..

Comment 32

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 33

[Diane Sun]

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

Comment 34

[(not reading, please use [e-mail] instead)]

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

Comment 35

[(not reading, please use [e-mail] instead)]

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

Comment 36

[(not reading, please use [e-mail] instead)]

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.)

Comment 37

[(not reading, please use [e-mail] instead)]

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

Comment 38

[(not reading, please use [e-mail] instead)]

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.

Comment 39

[(not reading, please use [e-mail] instead)]

Attachment: fix js exceptions.

Comment 40

[Diane Sun]

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

Comment 41

[Diane Sun]

fix checked in.

Comment 42

[(not reading, please use [e-mail] instead)]

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

Comment 43

[Diane Sun]

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

Comment 44

[grylchan]

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)

Comment 45

[(not reading, please use [e-mail] instead)]

> 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.

Comment 46

[grylchan]

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

Comment 47

[grylchan]

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.