User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2 Build ID: 20110902133214 Steps to reproduce: I compacted folders, then also ran the Xpunge add-on to do multiple. The is probably relevant to all or any version. Actual results: Doing a single folder is fine, I know what I tried to compact and the message in the status bar says "compacting folder".... and you wait. Great, I know why I'm waiting, but when I use the Xpunge add-on, it does multiple, and I have no idea where and what it is doing. Expected results: I think it would be great to add to the status bar, what folder AND for what account was being compacted. Just saying "compacting inbox" does no good if I have 7 of them.

invalid? File | compact folders shows folder names in status bar, so I think the fault here is the addon

Dunno sometimes on imap you choose compact and you get plenty of folders that get compacted (ie very true with gmail accounts).

Compact big folder. Actual result: Status text: Compacting folder Inbox... wonder which of your 5 inbox folders this might be expected result Status text: Compacting folder Inbox of john.doe@asdf.com... More possible improvements: a) Status text: Compacting folder Inbox of john.doe@asdf.com [30% completed]... b) when other status messages come in the way, e.g. automatic download of msgs, and compacting still continues after that, re-display the compacting status text (instead of just showing progress bar and nobody knows what and where we are progressing) If there is no duplicate, this should be confirmed.

Split this out of bug 66860 to make the chunks of work manageable.

This is still to be done. Probably use the format used for the messages in other account types: "account name: message".

Possible fix.

Comment on attachment 8340625 [details] [diff] [review] Patch v1 Review of attachment 8340625 [details] [diff] [review]: ----------------------------------------------------------------- Can you just do the account name prepending in nsFolderCompactState::ShowStatusMsg ? If necessary, pass the folder into it so it can lookup the server and account name. Be sure it does the right thing when called as ShowStatusMsg(EmptyString()) :)

Moved the code in ShowStatusMsg()

Comment on attachment 8340666 [details] [diff] [review] Patch v2 Review of attachment 8340666 [details] [diff] [review]: ----------------------------------------------------------------- This does work for me. Thanks. ::: mailnews/base/src/nsMsgFolderCompactor.cpp @@ +246,5 @@ > + nsCOMPtr<nsIStringBundleService> sbs = > + mozilla::services::GetStringBundleService(); > + nsCOMPtr <nsIStringBundle> bundle; > + rv = sbs->CreateBundle("chrome://messenger/locale/messenger.properties", > + getter_AddRefs(bundle)); When you set rv you should also check it, as above.

Thanks for the feedback.

Comment on attachment 8342865 [details] [diff] [review] Patch v2.2 >+ nsString statusMessage; Nit: move this nearer its use. >+ nsresult rv = m_folder->GetServer(getter_AddRefs(server)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ server->GetPrettyName(accountName); >+ nsCOMPtr<nsIStringBundleService> sbs = >+ mozilla::services::GetStringBundleService(); >+ nsCOMPtr <nsIStringBundle> bundle; >+ rv = sbs->CreateBundle("chrome://messenger/locale/messenger.properties", >+ getter_AddRefs(bundle)); >+ NS_ENSURE_SUCCESS(rv, rv); This code belongs with the other new code block below. Also, MSGS_URL. >+ const PRUnichar *params[] = { accountName.get(), aMsg.get() }; [This is char_16t these days.] >+ bundle->FormatStringFromName(NS_LITERAL_STRING("statusMessage").get(), >+ params, 2, getter_Copies(statusMessage)); And this is now MOZ_UTF16(). >+ return statusFeedback->SetStatusString(statusMessage); [Huh, I wonder why this one uses SetStatusString and everywhere else uses showStatusString...]

I hope this suffices. But.. does this patch actually work? SetStatusString(), the pages say, is used for making the status bar message persist until next user action.

This is how the message in statusbar looks. The only weird is, it says "No new messages on the server" but in screenshot you can see 4 new (unread) messages. But this could be something for a new bug.

Comment on attachment 8362609 [details] screenshot Sorry, wrong bug.

Can we implement this inside nsMsgStatusFeedback which already caches the messenger.properties bundle and already contains the needed code? Like have a new SetFormatStatusString function that prepends the account and then calls SetStatusString ? The prepending would be moved from onStatus to a shared function inside nsMsgStatusFeedback. The same could be done for ShowStatusString in bug 944526. Neil, would it be acceptable to somehow add new functions to nsMsgStatusFeedback ? (In reply to neil@parkwaycc.co.uk from comment #11) > >+ return statusFeedback->SetStatusString(statusMessage); > [Huh, I wonder why this one uses SetStatusString and everywhere else uses > showStatusString...] The js implementations of setStatusString and showStatusString in mailWindow.js are slightly different. Are those implementations called from the C++ code?

(In reply to aceman from comment #15) > Neil, would it be acceptable to somehow add new functions to > nsMsgStatusFeedback ? It could be done, but I can't think of a straightforward way of achieving it. > (In reply to comment #11) > > >+ return statusFeedback->SetStatusString(statusMessage); > > [Huh, I wonder why this one uses SetStatusString and everywhere else uses > > showStatusString...] > > The js implementations of setStatusString and showStatusString in > mailWindow.js are slightly different. Are those implementations called from > the C++ code? The JS versions are called from the C++ versions. This is done because the C++ object is used by IMAP, and so might be released on the IMAP thread. (But the rest of the code all happens on the main thread.)

So, which bug is going to contain the cleanup that we intend to do? If this bug is the one i.e. I am supposed to make changes to this patch, please let me know.

Can we finish the other bugs sharing what it already done (in nsMsgStatusFeedback) and then we will see what remains to be done for this one? Maybe we need to also do some changes for the other bugs (server types). I'd say this particular (compacting) message has the lowest priority.

Comment on attachment 8362605 [details] [diff] [review] Patch v2.5 (Haven't actually tested this.)

(In reply to :aceman from comment #18) > Can we finish the other bugs sharing what it already done (in > nsMsgStatusFeedback) and then we will see what remains to be done for this > one? Maybe we need to also do some changes for the other bugs (server > types). I'd say this particular (compacting) message has the lowest priority. I think now we can decide what else needs to be done here. Thanks.

I'm not privy to the plans here, but what is the way forward?

Considering comment 16, let's not drag this further. I have tested it and it works on compacting folders. Neil already gave it r+. But I have rewritten the error checking to make failures in fetching the account name able to be skipped over and the original message printed. So I'd like Jorg to check it lightly.

Comment on attachment 8851285 [details] [diff] [review] Patch v2.6 Review of attachment 8851285 [details] [diff] [review]: ----------------------------------------------------------------- Please add a comment where indicated. One unrelated thing. I compact folders a lot. And most of the time, the message is only displayed for a split second so I don't see how much disk space I'm saving. On rare occasions, the status message stays for a while so I can read it. Can that be improved? ::: mailnews/base/src/nsMsgFolderCompactor.cpp @@ +345,5 @@ > + params, 2, getter_Copies(statusMessage)); > + if (NS_FAILED(rv)) > + break; > + } while (false); // error management > + return statusFeedback->SetStatusString(statusMessage); I don't understand this code. You have an do {} while (false); and break out of that when something doesn't work. Excellent, Kent's favourite. But afterwards, you never check the status. In this case, you use the original value of 'statusMessage' which is initialised to aMsg: nsString statusMessage(aMsg); If that's the desired behaviour, there is a comment missing ;-)

(In reply to Jorg K (GMT+1) from comment #23) > One unrelated thing. I compact folders a lot. And most of the time, the > message is only displayed for a split second so I don't see how much disk > space I'm saving. On rare occasions, the status message stays for a while so > I can read it. The saved space should be shown as the last message and persist in the status bar for a while. But yeah, I have also seen while testing this, that it was immediately cleared. Probably by a message about loading a message inside the folder. > Can that be improved? I don't know, but not in this bug. > ::: mailnews/base/src/nsMsgFolderCompactor.cpp > @@ +345,5 @@ > > + params, 2, getter_Copies(statusMessage)); > > + if (NS_FAILED(rv)) > > + break; > > + } while (false); // error management > > + return statusFeedback->SetStatusString(statusMessage); > > I don't understand this code. You have an do {} while (false); and break out > of that when something doesn't work. Excellent, Kent's favourite. > > But afterwards, you never check the status. In this case, you use the > original value of 'statusMessage' which is initialised to aMsg: nsString > statusMessage(aMsg); What status? If any break is taken, we use the original aMsg. But I see the last break is strange. If the last rv is failure we don't know what statusMessage may contain. We should reset it back.

Comment on attachment 8851295 [details] [diff] [review] Patch v2.7 Review of attachment 8851295 [details] [diff] [review]: ----------------------------------------------------------------- > What status? Sorry, I was referring to 'rv' as "status". Anyway, let's shift it around a bit to make it clearer. Thanks for the patience. ::: mailnews/base/src/nsMsgFolderCompactor.cpp @@ +328,5 @@ > + > + nsCOMPtr<nsIMsgIncomingServer> server; > + nsAutoString accountName; > + // Try to prepend account name to the message. > + // If fetching any of the needed info fails, just show the original msg. Move this comment down. @@ +329,5 @@ > + nsCOMPtr<nsIMsgIncomingServer> server; > + nsAutoString accountName; > + // Try to prepend account name to the message. > + // If fetching any of the needed info fails, just show the original msg. > + nsString statusMessage(aMsg); I have a better idea: nsString statusMessage(); @@ +345,5 @@ > + const char16_t *params[] = { accountName.get(), aMsg.get() }; > + rv = bundle->FormatStringFromName(u"statusMessage", > + params, 2, getter_Copies(statusMessage)); > + if (NS_FAILED(rv)) > + statusMessage.Assign(aMsg); remove this, no error checking, fall out of the loop. @@ +346,5 @@ > + rv = bundle->FormatStringFromName(u"statusMessage", > + params, 2, getter_Copies(statusMessage)); > + if (NS_FAILED(rv)) > + statusMessage.Assign(aMsg); > + } while (false); // error management Here you put: // If fetching any of the needed info failed, just show the original message. if (NS_FAILED(rv)) statusMessage.Assign(aMsg); Then at the call site it is 100% clear which argument is used and one doesn't have to read back.

OK, that should work too.

(In reply to :aceman from comment #24) > (In reply to Jorg K (GMT+1) from comment #23) > > One unrelated thing. I compact folders a lot. And most of the time, the > > message is only displayed for a split second so I don't see how much disk > > space I'm saving. On rare occasions, the status message stays for a while so > > I can read it. > > The saved space should be shown as the last message and persist in the > status bar for a while. > But yeah, I have also seen while testing this, that it was immediately > cleared. Probably by a message about loading a message inside the folder. I have found out you can see the message if you are viewing a different folder than you want to compact. Then rightclick the wanted folder and compact it. Message about space saved would persist as the current folder hasn't changes so is not reloaded (with a status message).

https://hg.mozilla.org/comm-central/rev/92ed8653017e7e7ae522997a1d9b0607282b4bfb (took the liberty to add a blank line after the do-while block) Aceman, thanks for your contribution! ;-) (landed two patches of new contributors in the same push, so repeating the message here)

And thanks to Thomas for poking it!!

Oops, and of course thanks to Suyash!