79239 - Offline UI: Select Folders for Download shouldn't list POP and Local Folders

Status: VERIFIED FIXED

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

Description

[scottputterman]

Using 2001050704 on Win NT.

I know that 4.x used to list local folders but now that we have multiple
accounts and treat imported accounts as local folders I don't think we should be
showing Local Folders or POP accounts in the Select Folders for Download dialog.

Comment 1

[scottputterman]

nominating and reassigning

Comment 2

[Diane Sun]

From the spec,  it only requires not showing the AOL accounts.
And 4.x  shows local folder.  Should we have some consistency between 4.x and 
6.x?

Comment 3

[scottputterman]

I guess I'm not sure what value having folders that can't be selected for
offline adds to the dialog.  I might be missing something so if anyone knows I
don't mind being informed.

Comment 4

[jglick]

I agree it would be nice if POP and Local didn't show in the dialog.

Comment 5

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

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

Comment 6

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

we need to tweak the template rules in msgSelectOffline.xul and fix the folder 
datasource to pass through that certain folders are not "offline folders".

I think we can do this by returning false if the offline level for the folder's 
server is <= 0.  (-1 is undefined, 0 is no support, I think)

this would prevent local folders and pop folders from showing up in this dialog.

we could then have an boolean attribute on nsIMsgFolder for 'is offline folder"
that we could use for disabling / enabling certain ui elements based on the 
folder pane selection.

Comment 7

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

taking.  this will be all in the core of mailnews and I'd like to do the work.

It shouldn't take me long at all, I hope to have something for very early in 
0.9.2 so that we can start using this to fix other parts of the UI and cleanup 
some of our code.

Comment 8

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

p1, I want to fix this early to unblock monahb and dianesun

Comment 9

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

to clarify, this bug is about the "select folders" dialog.  but to fix that 
properly, I'm going to do makes some changes to the back end so that we can fix 
this dialog and the "folder properties" dialog (see #71128) in the correct way.

Comment 10

[laurel]

So, Seth:  this work here would also prevent AOL accounts from being shown in
the select items dialog, too?  Let me know if I need a separate bugscape bug...
(hmm, maybe I already logged one, I'll look.)

Comment 11

[scottputterman]

adding PDT+

Comment 12

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

> So, Seth:  this work here would also prevent AOL accounts from being shown in
> the select items dialog, too?  Let me know if I need a separate bugscape bug...
> (hmm, maybe I already logged one, I'll look.)

it should, depending on the offline level we set for those accounts.  I'll work
with racham to make sure that works.

Comment 13

[Håkan Waara]

Fixing this would also fix the folderpane_outliner regression bug 85088, and
since this is targeted for 0.9.2 - ain't that just great? :)

Comment 14

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

fix in hand.

the follow fix make it so only servers (and folders) that support offline show 
up in the dialog.  I may need to tweak the threshhold so that AOL imap servers 
don't show up.

this patch also fixes the problem where "Offline Settings" was showing up in 
account central for pop servers, when it shouldn't, right?  

I hope to get a review / discuss the fix with racham today.

Comment 15

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

Attachment: fix, version 1.

Comment 16

[Håkan Waara]

looks like solid code to me, r=hwaara.

Comment 17

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

I forgot something.  I wanted to add supportsOffline to nsIMsgFolder, because 
we'll want to use that to enable / disable some offline menu items in the three 
pane.

new patch coming up.

Comment 18

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

Attachment: new patch, provides supportsOffline attribute on nsIMsgFolder

Comment 19

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

note to laurel / bhuvan:  looking at mailnews-ns.js, the default offline level 
for AOL imap is OFFLINE_SUPPORT_LEVEL_NONE, and the default offline level for 
netcenter is OFFLINE_SUPPORT_LEVEL_REGULAR

those defaults and this patch will make it so AOL folders / servers will not 
show up in this dialog, but netcenter will.  

is that desired behaviour?

Comment 20

[laurel]

Yes. AOL should not show, Webmail accounts should.

Comment 21

[racham]

Yes Seth. As mentioned by laurel also, it is the desired behavior.

You know the side effect of this is that we make Offline & Disk Space item 
disappear in the AccountManager window under pop Accounts. But we do have some 
disk space settings for POP listed in the panel for this item today. So, there 
is a need to move those disk space prefs to the pop server panel (am-server.xul) 
before we land this patch. That is the only thing I am concerned about. 
Otherwise, all the changes look good. 

As the matter of fact, we need to move only one pref (of the two that are 
there). 

If you look at bug 81494, you will notice that we are trying to keep the pref in 
offline panel and get rid of the one in server panel. Now because of this change 
you may have to keep the one in server panel. 

Then that leaves us with the second pref "Compact folders when it will save over 
X kb". Diane/Navin, isn't this the one that is moved to the global offline 
panel ? If so, then we are clean.

bhuvan

Comment 22

[racham]

Small corrections to my last update to make things more clear : 

* As mentioned by laurel also, it is the desired behavior ==> As mentioned by
laurel also, it is the desired behavior for AOL and Webmail accounts.

* You know the side effect of this ==> You know the side effect of this patch

bhuvan

Comment 23

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

> You know the side effect of this is that we make Offline & Disk Space item 
> disappear in the AccountManager window under pop Accounts. But we do have 
> some disk space settings for POP listed in the panel for this item today. 

ah, I understand.  thanks for catching that.

we should not be using the offline level attribute for disk space.

since that panel is for both offline AND disk space, I can add another 
attribute to nsIMsgIncomingServer for supportsDiskSpace.

then, in nsMsgAccountManagerDS.cpp (around line 541), I can check 
supportsDiskSpace in addition to offlineSupportLevel.

comments?  (I'll work on a new patch while I wait for feedback.)

Comment 24

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

related comments:

1)  in account central, it should say "Offline & Disk Space Settings" 
under "Advanced Features" for imap, but for pop, it should say "Disk Space 
Settings".  my new patch should allow us to do that (by using the per server 
supportsDiskSpace attribute.  this is bug #83594

2)  in account manager, for pop, the panel should be "Disk Space" instead 
of "Offline & Disk Space".  I think my new patch should allow for that fix, too.

my new patch is on the way soon.  I'll see if I can fix those two problems all 
in the same patch.

Comment 25

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

ok, I've got a fix in hand for this bug, and #83594

this patch I'm about to attach makes it so:

1)  when selecting offline folders, only folders that support offline show up.  
2)  under advanced features in "accout central", "offline settings" will not 
show up for pop (this is bug #83594)
3)  in account manager, "offline & diskspace settings" does show up for pop.

staying with the spec, I didn't make it so the title was "diskspace" (when 
there is no offline, like for pop) but I see what it would take.  (jglick, if 
you want this, please update the spec and log a bug on me.)

again, staying with the existing spec, I also didn't make it so in account 
central, under advanced features, it would have "disk space settings" for 
accounts with disk space settings, but not offline settings (like for pop.)  
that would have been more work, and again, I'd jglick to update the spec / log 
a bug if she wants that.

here comes the patch, please review again.

Comment 26

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

Attachment: patch, v3. (add and uses supportsDiskSpace attribute on nsIMsgIncomingServer.)

Comment 27

[racham]

You may have to override this function in nsImapIncomingServer to handle AOL as
a special case (returning false). Otherwise, the following is going to make AOL
accounts to show Offline & Disk Space item in AccountManager.

 NS_IMETHODIMP
+nsMsgIncomingServer::GetSupportsDiskSpace(PRBool *aSupportsDiskSpace)
+{
+    NS_ENSURE_ARG_POINTER(aSupportsDiskSpace);
+    *aSupportsDiskSpace = PR_TRUE;
+    return NS_OK;
+}
+

Let me know if I missed something.

bhuvan

Comment 28

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

good catch.  I'll extend the fix so that AOL mail won't show the panel.  (the
fix will be similar to how the offline level can be overriden per domain.)

Comment 29

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

once I finish and land, I can clean up the fix to 
http://bugzilla.mozilla.org/show_bug.cgi?id=79267

Comment 30

[jglick]

OK, to summarize, is this correct?

For POP accounts we would move the pref related to disk space on the server 
setting panel to the "Offline & Disk space" panel to keep the two disk space 
related prefs together.

For IMAP and NNTP accounts "Offline" appears on Account Central but for Pop 
Accounts it does not.

>I didn't make it so the title was "diskspace" (when there is no offline, like 
>for pop) but I see what it would take.  

Seth, it would be great if you could do this. I will fill a bug.
http://www.mozilla.org/mailnews/specs/accounts/images/AcctOffPop1.gif

>I also didn't make it so in account central, under advanced features, it would 
>have "disk space settings" for accounts with disk space settings, but not 
>offline settings (like for pop.) 

This is correct.  

Comment 31

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

> OK, to summarize, is this correct?
>
> For POP accounts we would move the pref related to disk space on the server 
> setting panel to the "Offline & Disk space" panel to keep the two disk space 
> related prefs together.

yes.  except that it never moved to the server settings panel.  is that some 
other bug?  

> For IMAP and NNTP accounts "Offline" appears on Account Central but for Pop 
> Accounts it does not.

yes.  (except for AOL imap accounts, which do not support offline)

> Seth, it would be great if you could do this. I will fill a bug.
> http://www.mozilla.org/mailnews/specs/accounts/images/AcctOffPop1.gif

ok, no problem.  I've got that fix in hand and it will be included in this fix.

>>I also didn't make it so in account central, under advanced features, it 
would 
>>have "disk space settings" for accounts with disk space settings, but not 
>>offline settings (like for pop.) 
>
>This is correct.  

I got confused here.  to clarify, there will not be a "Disk Space" link in 
account central for pop accounts.

I'll attach a new patch, and some screen shots.

Comment 32

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

quick question about 
http://www.mozilla.org/mailnews/specs/accounts/images/AcctOffPop1.gif

you have "Disk Space - <acc name>"

in the current UI, we don't have "- <acc name>" for any of the panel titles.

can you open up a seperate bug on that?

Comment 33

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

Attachment: account manager, for pop server

Comment 34

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

Attachment: account manager, for imap server

Comment 35

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

Attachment: account central for pop

Comment 36

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

Attachment: account central for imap

Comment 37

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

Attachment: select folder dialog, notice no pop, no local folders

Comment 38

[jglick]

>yes.  except that it never moved to the server settings panel.  is that some 
>other bug? 

sorry, it was originally in Both places.

>(except for AOL imap accounts, which do not support offline)

right. :-)

>to clarify, there will not be a "Disk Space" link in account central for pop 
>accounts.

Correct.

Disk Space vs Offline and Disk Space - Bug 85964 

>you have "Disk Space - <acc name>"
yeah, ignore that for now. separate issue.

Comment 39

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

"Local Folders", since the folders are on disk, should have a "Disk Space" 
panel, right?

it's easy to fix nsMsgAccountManagerDS.cpp to make "disk space" show up for 
local folders in the account settings dialog, but am-offline.xul / .js will 
need some work to support a server of type "none".

instead of using server type, we should switch to checking attributes on the 
server.  there is a bug for that already.  I'll log a new bug on making "Disk 
Space" show up for "Local Folders".

Comment 40

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

Attachment: mozilla part of the fix.

Comment 41

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

Attachment: ns part of the fix, for the issue racham pointed out with AOL mail

Comment 42

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

the patch fixes the following problems:

1)  accounts that don't support offline will not show up in the select folders 
dialog

2)  accounts that don't support offline (like pop) will not have "Offline 
Settings" in account central

3)  accounts that don't support offile, but do support disk space, will 
have "Disk Space" in account settings.

4)  server.supportsDiskSpace and folder.supportsOffline are now available for 
use to clean up our existing .js

Comment 43

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

and 

5) the offline support level for pop3 servers is properly set at NONE.  

#17204 is blocked by that.

Comment 44

[racham]

Patch looks good to me. Good to see the generalization you did in
nsImapIncomingServer to create prefnames with host names.

r=bhuvan

Comment 45

[Scott MacGregor]

sr=mscott

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

a=dbaron for trunk checkin (on behalf of drivers)

Comment 47

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

fixed.

Comment 48

[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

Couple of comments and/or questions.

1)In both MAC and Linux builds: I don't see Disk Space or Offline
  and Disk Space in the 'Account Settings' window (left hand window).

2)For Windows builds: I do see Disk Space in the 'Account Settings' window 
  (left hand window). Is it supposed to display the same GUI as Offline &
  Disk Space for Imap account? The only thing that is different between the 2 
  GUI's is the tile bar (imap says Offline & Disk Space) (AOL mail says
  Disk space). AOL mail's right hand panel for the first pref says "offline" 
  and includes a 'select button')

I am assuming Title heading 'Disk Space -<acc name>' didn't make it in
this patch?

Not sure if I should reopen this bug based on Disk Space not showing
up on Mac/Linux builds?

Can include screen shots, if needed.

 

Comment 49

[jglick]

>I am assuming Title heading 'Disk Space -<acc name>' didn't make it in
>this patch?

Correct. None of the Acct Setting titles are using this yet (so ignore for now).

IMAP accounts (regardless of platform) should have "Offline & Disk Space"
POP accounts should have "Disk Space".
AOL accounts, not sure. We don't do any offline stuff for them currently. Any 
disk space stuff?

Comment 50

[grylchan]

oops should be clearer

>1)In both MAC and Linux builds: I don't see Disk Space or Offline
>  and Disk Space in the 'Account Settings' window (left hand window).

This is for AOL accounts only. For AOL accounts I see Server Settings,
Copies and Folders, and Addressing. No Disk Space/Offline & DiskSpace
prefs for AOL accounts. Again this happens only on Linux/Mac builds.

All other accounts are properly displaying
Diskspace or Offline & Disk Space.

So my question is how AOL accounts are/should be handled? And whether
this bug should be reopened or new bugs in bugscape should be filed?

Comment 51

[timeless]

AOL related bugs should be in bugscape.

Comment 52

[laurel]

We already know about AOL bugs and bugscape -- back off. The AOL part of this
discussion followed from the overall problem.

Since this bug has gotten way too confusing, and since we have some conflicting
behavior in the past couple commercial trunk builds regarding the AOL offline
display, I logged bugscape bug 6657 to get this nailed down before RTM finalization.

Comment 53

[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

In verifying, I am only going to concern myself with the
following: imap, pop, web, local folders, and newsgroups. I will
put in comments about aol mail but won't let that
deter me from verifying this bug.

Verified the following:

  -In "Select Items for Offline Usage" GUI, the following accounts appear:
   Imap, web mail, and newsgroups
  -In Account Central, under Advanced Features:
    * Offline settings doesn't appear in pop, AOL, and Local folders
    * Offline settings does appear in Imap, web mail, and newsgroups
  -In the Account Settings "Tree" (left side window):
    * Disk Space appears only for POP accounts
    * Local Folders and AOL mail don't have 'Disk Space' or 'Offline & 
      Disk space'
    * Offline & Disk Space appears for imap, webmail, and newsgroups

The weirdness I saw "For Windows builds: I do see Disk Space in the 'Account 
Settings' window" is not there anymore. On all platforms, for aol accounts,
there is no disk space or offline & disk space setting. 

Hopefully I covered everything Seth fixed. :-)

Marking as verified. 
   

Comment 54

[grylchan]

Note that bug 83594 had fallen through the cracks. It
dealt with offline settings for pop accounts in account central.
Since that was fixed & verified in this bug that bug has
since been marked verified.