142748 - File/Compact Folders disabled until you focus a folder

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Boris 'pi' Piwinger]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0+) Gecko/20020503

In the 3pane when you select an account (POP3 in my case) Compact Folders in the
File menu is greyed out. I have to select a folder for that account to get it
working. It should be available whenever I am in an account (even on the start
page for that account).

pi

Comment 1

[Boris 'pi' Piwinger]

Looks similar to the problem in bug 111102.

pi

Comment 2

[Troodon]

I can confirm this for 2002091014 and Win98 SE. It's disabled only when the
account name is selected.

Comment 3

[Mike Cowperthwaite]

Still true in 1.5RC1.  Updating platform.

xref bug 93001.

Comment 4

[Mike Cowperthwaite]

I've poked around at this a bit, and I think I can patch it, but I'm confused 
about some things.

In mailWindowOverlay.js, the function IsCompactFolderEnabled() performs a test 
on:
  (GetFolderAttribute(folderTree, folderResource, "CanCompact") == "true")

Now, it turns out that for 'server' nodes in the tree, 'CanCompact' is always 
set to false, but for local folders it's true.  (For news folders, it's false; 
for IMAP folders, I presume it's true.)

1) Why is GetFolderAttribute() being called?  All the folderResource objects 
I've looked at have CanCompact as a member.  If there's some remote chance that 
the member won't be present, wouldn't something like this be cleaner? :
  try {
    canCompact = folderResource.canCompact;
  } catch () { canCompact = false; }
I notice, by contrast, that MsgCompactFolder() [in widgetglue.js] makes a direct 
access to resource.server, with no protection.


2) Where is this 'canCompact' member being set?  And more importantly, why is it 
being set to 'false' for servers?  Is this just how it's always been, or am I 
trying to bypass some safety concern here?

If this is just how it's always been, it seems to me the right thing to do is to 
initialize that value to 'true' for non-news servers, rather than patching the 
javascript.

3) I hacked together a quick-and-dirty JS patch and tried it, and when I ran the 
Compact Folders with the server root selected, it looked like it ran OK.  
However, I tried to follow the code (via LXR) thru the call to 
MsgCompactFolder() but got lost after finding [in nsMessenger.cpp]
nsMessenger::CompactFolder().  My concern is that by passing CompactFolder() the 
resource to a server rather than a folder, something will go wrong down where I 
haven't been able to find anything yet.

4) Is there any sort of folder or server that *can't* be compacted, other than 
news accounts and news groups?  I'm pretty unfamiliar with IMAP, so that's my 
major concern.

Comment 5

[Mike Cowperthwaite]

Attachment: Patch (-u against 1.6 Final)

This works nicely.  Added code to explicitly disable File|Compact for news
folders and accounts, and to otherwise enable File|Compact for servers.  Also
took out the clunky re-examination of the folder tree -- since we already have
the URI of the folder node, we can get the resource much more simply.

Regarding my questions in comment 4:  (1) yes, GetFolderAttribute() is
necessary, as I discovered; (2) 'canCompact' apparently means "has some file
representation which can be compacted" which clearly doesn't apply directly to
accounts/servers; (3) when the menu action actually passes the server/account
to MsgCompactFolder(), this seems to work without a problem -- but this is
something worth reviewing; (4) I opened an IMAP account to check my patch, and
it works as expected; in particular, the Compact Folders menu item is disabled
for the IMAP account if offline, otherwise not.

Comment 6

[David :Bienvenu]

Comment on attachment 143510 [details] [diff] [review]
Patch  (-u against 1.6 Final)

you should be able to compact the offline store of a news folder by selecting
the folder and doing a compact folder. I'm not sure it works, but I would
rather leave it enabled, unless, perhaps, there is no offline store for the
newsgroup...

Comment 7

[Mike Cowperthwaite]

Attachment: Revised patch -u against 1.7-0310

Like this?
I've made the logic a little wordier because the |GetFolderAttribute()!="true"|

clauses are just too hard to keep straight.

Comment 8

[David :Bienvenu]

Comment on attachment 143537 [details] [diff] [review]
Revised patch    -u against 1.7-0310

sure, thx.

Comment 9

[Mike Cowperthwaite]

Comment on attachment 143537 [details] [diff] [review]
Revised patch    -u against 1.7-0310

Neil, if this looks OK to you, could you check it in for me?

Comment 10

[[e-mail]]

OK, so you need to check isCommandEnabled, this basically tells you if you're
trying to compact an IMAP server while offline, which isn't supported.

But the canCompact flag really applies to Compact This Folder (on the folder
pane context menu). It's the equivalent of !isServer && !isNews - only
autocompaction of news offline stores appears to be supported, afaik.

There is of course the canCompactFoldersOnServer, but I'm not sure that this is
useful or relevant, so I would be happy with a simple !isNews check there.

Comment 11

[Mike Cowperthwaite]

Attachment: Another revision diff -u against 1.7-0310

Per discussion with David, this patch now enables File|Compact Folders for news
servers and groups, as well for the servers for POP, IMAP and Local Folders.
|canCompactFolderOnServer| is only checked for IMAP -- in the news case, it is
conceivable that |canCompactFolderOnServer| will be (properly) set to false,
and we wouldn't want to disable the local operation ("apply retention settings"
plus, in the future, compaction of the offline store).

Comment 12

[[e-mail]]

Comment on attachment 143710 [details] [diff] [review]
Another revision    diff -u  against  1.7-0310

You do know that you should have the r and sr the other way around (assuming
here that bienvenu's r= counts as sr=, which I checked, and it does). So r=me.

Comment 13

[Mike Cowperthwaite]

Comment on attachment 143710 [details] [diff] [review]
Another revision    diff -u  against  1.7-0310

Asking for approval for this low-risk patch.

Comment 14

[chris hofmann]

Comment on attachment 143710 [details] [diff] [review]
Another revision    diff -u  against  1.7-0310

a=chofmann for 1.7b

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[Mike Cowperthwaite]

I've added the issue of the text for "Compact Folders" being not quite accurate 
for news accounts to bug 94251.

Comment 17

[Boris 'pi' Piwinger]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040407

Verifying.

pi

Comment 18

[Mike Cowperthwaite]

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