Closed Bug 17204 Opened 20 years ago Closed 19 years ago

[FEATURE] mail.purge_threshhold "Automatically compact folders..."

Categories

(MailNews Core :: Backend, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: laurel, Assigned: naving)

References

()

Details

(Whiteboard: [nsbeta1+]have fix)

Attachments

(8 files)

This is a bug to track implementation of the feature to automatically compact
folders upon a user-specified threshhold.

Currently the associated prefs mail.prompt_purge_threshhold and
mail.purge_threshhold do migrate with the proper values to the prefs.js file.

The Prefs UI doesn't currently work to write these changes to the prefs.js file
(covered under bug #17201).
QA Contact: lchiang → huang
Target Milestone: M14
Status: NEW → ASSIGNED
Target Milestone: M14 → M16
Whiteboard: ETA 05/09/00
This sounds like a fair amount of works. 
Whiteboard: ETA 05/09/00 → 5 days
Hey Jeff, since we are past feature freeze and this isn't on the beta2 list,
should we move this off to a future release? either that or we need to nominate
it for beta2.
Future release, M20....
Target Milestone: M16 → M20
reassigning jefft's bugs to naving
Assignee: jefft → naving
Status: ASSIGNED → NEW
Nominating for beta - also need this for cleaning up offline storage as well.
Are we planning on doing this post beta1?
Keywords: nsbeta1
Yeah, we should look into this.  
Priority: P3 → P2
Whiteboard: 5 days → [nsbeta1+]5 days
Target Milestone: --- → mozilla0.9.2
SPAM: please implement this before all our hard drives explode ;)
moving to 0.9.3.  We should remove the UI for this until this gets implemented.
I will file a bug.
Target Milestone: mozilla0.9.2 → mozilla0.9.3
scott, I was working on this and will have a fix soon. 
Target Milestone: mozilla0.9.3 → mozilla0.9.2
ok, but if it looks like it's going to take a long time we should put this back
in 0.9.3
Attached patch proposed fix, v1Splinter Review
The fix is to check if the pref is checked off and if it is checked, then check 
the number of expunged bytes at intervals of 1 hr. If the total expunged 
bytes exceed the limit, compact all folders. 

cc david for review. 
initial comments:

need newline at end of local/resources/locale/en-US/localMsgs.properties

what's 36000000000000? Need a comment or a #define

This doesn't do anything for compaction of offline news or imap stores (which is
going to be a bit of work that I'll need to talk to you about).
The attached patch makes it so that imap and news db's will keep track of
expunged bytes as the total byte size of offline message bodies whose msg hdrs
were deleted. This will make expungedBytes for news and imap folders correct,
except that existing imap and news db's will have bogus counts because up til
now, they've been counting every msg hdr delete as expunged bytes. It would be
good to fix this, but not absolutely neccessary since it will just cause an
initial compaction of offline stores.

There are more details to deal with for offline. CompactAll will need to compact
offline stores, or we'll need to add a call to compact offline stores as well.
The method to compact an offline store is CompactOfflineStore(). If this method
doesn't set the expunged bytes to 0 for a folder, I'll need to fix that. Cc'ing
Seth so we can start getting reviews for these patches.
How much work do you think is left on this?  David mentioned that doing this for
offline is going to be a bit of work.  If it's a lot of work I think we should
wait on this (or just get the pop/local folders part in)
I don't think it's very much work - basically, instead of iterating over only
local folders, we need to iterate over imap and news folders if any of them are
configured for offline use.
the fix is that I have added a function AutoCompact() to nsMsgDBFolder which 
gets called when you UpdateFolder , local/news/imap. For imap and news it gets
called only when we are offline. So it prompts the user if he/she wants to 
compact all folders if they exceed limit. For offline just check if the folder
is for offline use. Start compaction if the user says OK! 
I am still trying to make it more modular and will attach a new patch. 
Attached patch proposed fixSplinter Review
So this new patch makes the folderCompactor more modular. request for review 
from david and seth. 
Whiteboard: [nsbeta1+]5 days → [nsbeta1+] have fix
adding PDT+, but we need to really test this before going in and make sure that
we don't know of any side effects. 
Whiteboard: [nsbeta1+] have fix → [nsbeta1+][PDT+] have fix
David, could you review the last patch. I have tested your 2nd patch and
compact offline works. r=naving on both your patches. 
are you intending that the purge threshhold should be global or per-server? I'm
not sure what the UI is looking like at this moment. It seems like you've made
it pretty easy to make it per-server, so maybe we should just go that way, in
which case you should use the macros for server prefs, and make sure the UI
people (Diane and Mohan) get and set those same prefs. Also, checking uri's for
"nntp" or "imap" is not the cleanest way of doing this - I think bhuvan was
going to add an attribute to the server which indicated whether it had offline
support, you should check into that or ask him. Sorry for forgetting to review
this - ping me if I don't get to a review in 24 hours, because if I haven't done
it in 24 hours, I've probably forgotten.
Purge_threshold is global right now 

#define PREF_MAIL_PROMPT_PURGE_THRESHOLD "mail.prompt_purge_threshhold"
#define PREF_MAIL_PURGE_THRESHOLD "mail.purge_threshhold"

but it should be on per-server basis, because the account manager wizard allows
you to set the boolean and limit on a per-server basis. 
It looks like these profiles will be on a global level (Edit | Preferences), 
dianesun/mohanb could you confirm ?
Yes,  they are global level preferences.
Could we have them as server-prefs. I think it is more logical and compact All 
folders also works on a per-server basis. 
Depends on: 79239
moving to 0.9.3
Whiteboard: [nsbeta1+][PDT+] have fix → [nsbeta1+]have fix
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Please make sure that the hourly autocompacting of on-/offline imap folders
doesn't remove messages that have only been *marked as deleted*. 

It would suck if someone marked a msg as deleted and the autocompact kicked-in
and permanently removed the msg, and then the user decides he wanted the file
after all :(
Depends on: 88984
Attached patch proposed fix, v3Splinter Review
I have made it such that we check for all the accts and see if the total
expunged bytes > purge_threshold (global pref), then we compact all local 
folders and also once the local folders are done, compact offline folders. On 
the way, found some problems so fixed those problems (like m_keyArray was not 
being cleared, m_messageUri was not being reset between compacts of folders) and 
it works for both local and offline. requesting reviews from david and seth.

Navin, will your fix delete msgs that were "marked as deleted"? If so, your fix
is not finished.
Peter, aren't you talking about imap messages/folders? This is only for local
folders, isn't it, Navin?
r=bienvenu
Yes, I am talking about IMAP folders - whose messages that are "marked as
deleted" ahould NOT be purged.

bienvenu, did you confirm with Navin that the patch doesn't BREAK this, before
you r='ed?
Peter, I don't see any code that will issue an online compact of imap folders.
This code is to compact the offline store. Do you have any evidence to back up
your claim that this breaks something, or are you just idly speculating?
... just "idly" speculating. Thank you for explicitly confirming that IMAP
folders are unaffected.
It will not affect your online imap stuff. 
1) interCaps

void StartCompactingAll()

should be

void startCompactingAll()

2) extra space in autoCompactAllFolders property

"threshold ?" should be "threshhold?"

3) can m_size ever be 0?

double check that, since if ever is zero, you'll crash.

4)  kStringBundleServiceCID

+static NS_DEFINE_CID(kStringBundleServiceCID, NS_STRINGBUNDLESERVICE_CID);

use the string bundle contract id instead of defining this static CID

5)  if promptPurgeThreshold and purgeThreshold are global prefs, don't add them
to the nsIMsgIncomingServer
interface.

you should remove this code:

+  readonly attribute boolean promptPurgeThreshold; 
+  readonly attribute long purgeThreshold;

+nsMsgIncomingServer::GetPromptPurgeThreshold(PRBool *aPrompt)
+{
...
+NS_IMETHODIMP
+nsMsgIncomingServer::GetPurgeThreshold(PRInt32 *aThreshold)

and fix this code to use prefs:

+       PRBool prompt;
+       server->GetPromptPurgeThreshold(&prompt);
+       PRInt32 purgeThreshold;
+       server->GetPurgeThreshold(&purgeThreshold);


is there UI for this?  from the code, it looks like it should be under the "Edit
| Preferences | Offline & Diskspace" panel, and would be global (a check box for
the bool pref, a text field for the threshold.)

6)  AutoCompact(aWindow);

you never check the return value of AutoCompact() (it returns a nsresult)
also, where'd you get that prompt string from?  if you made it up (and didn't
steal it from 4.x) you should run it by jatin@netscape.com (tech writer) or robinf
I have made changes for 1, 2 and 4. 3 is already taken care of because we 
check m_size for > 0 in StartCompacting(). 5 is a global pref and i have 
kept in the server because later on we might change it to be a server pref. 
for 6 the return value is not important. 
jatin, do you think this is correct text 

Do you wish to compact all local and offline folders because the total wasted 
space in all accounts exceeds the purge threshhold ?
> 5 is a global pref and i have kept in the server because later on we might 
> change it to be a server pref. 

that seems like a bad argument to me.  if later we make it a server specific
pref, we should change it then.  all you've done is add more code when we don't
need it.  the rest of your code goes through all servers and adds up the total
bytes (meaning the prefs are global, not per server)

your entire fix should be consistent.

it really depends on the UI.  it seems like the UI for this is (or is going to
be) global.

the spec doesn't seem to cover this (see
http://www.mozilla.org/mailnews/specs/prefs/Preferences.html)

jglick, can you confirm this will be a global, and not per server thing?
that's my fault about leaving the code per-server - Seth convinced me we should
just make it consistent, and the pref is global now, so the code should be global.
Attached patch proposed fix, v4Splinter Review
The patch size has increased because I have combined david's patch with my 
patch. seth, please review it again. 
1)

don't use static comptrs.  (see bug #85225, for example, on how this can cause
crashers)

static nsCOMPtr<nsISupportsArray> gOfflineFolderArray;

see http://www.mozilla.org/hacking/portable-cpp.html#dont_use_static_constructors

according to alecf:  "don't use static comptrs because the constructors might
not get called, plus it varies from platform to platform when the constructors
AND destructors will be called."

2)  use do_getService() and contract ids, instead of NS_WITH_SERVICE() and k*CID

NS_WITH_SERVICE(nsIMsgAccountManager, accountMgr, kMsgAccountManagerCID, &rv); 

3)

purgeThreshold*1000

shouldn't that be *1024 (it's kb, right?)

4)

+             PRUnichar *alertString;
+            
bundle->GetStringFromName(NS_ConvertASCIItoUCS2("autoCompactAllFolders").GetUnicode(),
+                            &alertString);

you leak alertString, and you don't need to use NS_Convert*().

do something like this:

nsXPIDLString alertString
bundle->GetStringFromName(NS_LITERAL_STRING("autoCompactAllFolders").get(),getter_Copies(alertString));
...
dialog->Confirm(nsnull, alertString.get(), &okToCompact);

5)

you still aren't checking the return result of AutoCompact();
Ok, so i will not use static nsCOMPtrs but instead i will pass the 
offlineFolderArray. All other changes also done. patch coming up...
Attached patch proposed fix, v5Splinter Review
you really want bienvenu to do the final review, as he understand this backend 
stuff more than I do.  (I can do the sr, though.)

questions:

1)  can you add a comment explaining what supportsArray (poorly named) is for?
maybe change the name so that a comment isn't necessary.  (aOfflineFolderArray 
is an array of all offline folders, right?)

void startCompactingAll(in nsISupportsArray supportsArray, in nsIMsgWindow 
aMsgWindow, in boolean compactOfflineAlso, in nsISupportsArray 
aOfflineFolderArray);

2)  casting to (void) and not checking rv?

+    (void)msgHdr->GetOfflineMessageSize(&size);
+    (void)msgHdr->GetMessageSize(&size);
+    (void)msgHdr->GetOfflineMessageSize(&size);

do we really not care about the return value?  if not, that implies we are 
expecting error in some cases and we want to proceed anyways.  if that is the 
case (I'm not sure if it is), you should add a comment.  

if error shouldn't happen, we should do something like:

rv = msgHdr->GetFoo()
NS_ENSURE_SUCCESS(rv,rv);
For 1) isn't the variable name aOfflineFolderArray self-explanatory. 
For 2) those changes are from david's patch. 
Seth is complaining about this line:
NS_IMETHODIMP nsFolderCompactState::StartCompactingAll(nsISupportsArray
*aSupportsArray

and was hoping you'd replace aSupportsArray with something like
aArrayOfFoldersToCompact or something like that.

Otherwise, it looks OK to me. r=bienvenu
fix checked in. Changed supportsArray to aArrayOfOfflineFoldersToCompact before
checking in. 
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
thanks for change it to aArrayOfOfflineFoldersToCompact

I gave a sr=sspitzer to naving over aim.
per esther reassigning to sheela
QA Contact: huang → sheelar
I spoke to esther as to how this bug needs to be verified. We understand that 
this fix is for the local and pop folders compaction. By checking the 
preference in offline and diskspace I should be prompted to compact local 
folders if it exceeds the size limit.  Here are some questions which I was not 
too clear on.

-Do we need to verify this bug both while online and offline for local folders? 
-Does this bug fix the time limit? Do we have any set time limit when the dlg 
has to appear if the folders need to be compacted again in the same session.  
-For example: When I select the pop account which needs to compact as per the 
preference and I click ok to the dlg and the folders are compacted. In the same 
session when I delete more messages  should I expect to be prompted with the dlg 
again? Does the compaction happen in the background without the dlg appearing?  
Or is it only once per session? 
-Should be this be tested by migrating a pop account from 4.x and checking the 
pref and size are migrated to 6.x?
-Does this involve any regression checks to be done with imap and news with 
offline?
Attempting to answer Sheela's questions:
1. Compaction should occurr both while on- and offline
2. There should be no time limit. If a folder needs to be compacted again, then
the user should be informed (that's what he set in his prefs). This shouldn't
happen too often and therefore a consisten behaviour is preferable (i.e.,
compact when threshold is exceeded).
3. A dlg and subsequent compaction should occurr every time it is needed. See #2
above.
4. Testing against 4.x: good idea.
5. Definetely checks are needed to make sure that IMAP is NOT compacted ("marked
as deleted"). For newsgoups: How are those "disk space" settings triggered (same
as this compaction? on load? etc.)?
As per my conversation with navin this bug does not matter if it online or 
offline
The compaction is triggered if it is required after 1 hr in the same session. 

verified this using 2001-09-12-05 trunk build on win98
still need to verify on mac and linux
verified on linux, mac- 2001-09-17-05 branch build. Fix was checked in july 
before the 0.9.4 branch so I guess this should be ok. 
Status: RESOLVED → VERIFIED
Forgot to mention checked the profile migration too when verifying this bug. 
If compaction only occurs after an hour; what about the many people that are
regularly online for less than an hour? 

Does the compaction also occurr at the *beginning* of a session?

If not, should we be reminding the user somehow that a compaction is overdue?
Compaction occurs at the beginning of the session if there is any room and then
at intervals of one hr. 
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.