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

This sounds like a fair amount of works.

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

reassigning jefft's bugs to naving

Nominating for beta - also need this for cleaning up offline storage as well. Are we planning on doing this post beta1?

Yeah, we should look into this.

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.

bug 83391 created to track UI removal.

scott, I was working on this and will have a fix soon.

ok, but if it looks like it's going to take a long time we should put this back in 0.9.3

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.

So this new patch makes the folderCompactor more modular. request for review from david and seth.

adding PDT+, but we need to really test this before going in and make sure that we don't know of any side effects.

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.

moving to 0.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 :(

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.

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

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.

thanks for change it to aArrayOfOfflineFoldersToCompact I gave a sr=sspitzer to naving over aim.

per esther reassigning to sheela

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.

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.