Improve status bar text when compacting folders: display account name, too ("Compacting folder Inbox..." with folder name only is ambiguous with multiple accounts)

RESOLVED FIXED

Status

MailNews Core
Backend
--
minor
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Al Knepper, Assigned: sshagarwal)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

7 years ago
invalid?
File | compact folders shows folder names in status bar, so I think the fault here is the addon
Severity: normal → minor
Summary: Enhancement request - additional info to display on screen → display folder being compacted in status bar
Whiteboard: [dupeme]
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.
Summary: display folder being compacted in status bar → Improve status bar text when compacting folders: display account name, too (folder name like inbox is ambiguous with multiple accounts)
Summary: Improve status bar text when compacting folders: display account name, too (folder name like inbox is ambiguous with multiple accounts) → Improve status bar text when compacting folders: display account name, too ("Compacting folder Inbox..." with folder name only is ambiguous with multiple accounts)
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

5 years ago
Blocks: 66860
No longer depends on: 66860

Comment 4

5 years ago
Split this out of bug 66860 to make the chunks of work manageable.
Assignee: nobody → syshagarwal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [dupeme]
Version: 3.1 → Trunk

Comment 5

5 years ago
This is still to be done. Probably use the format used for the messages in other account types:
"account name: message".
(Assignee)

Comment 6

5 years ago
Created attachment 8340625 [details] [diff] [review]
Patch v1

Possible fix.
Attachment #8340625 - Flags: feedback?(acelists)

Comment 7

5 years ago
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()) :)
Attachment #8340625 - Flags: feedback?(acelists) → feedback-
(Assignee)

Comment 8

5 years ago
Created attachment 8340666 [details] [diff] [review]
Patch v2

Moved the code in ShowStatusMsg()
Attachment #8340625 - Attachment is obsolete: true
Attachment #8340666 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #8340666 - Flags: feedback? → feedback?(acelists)

Comment 9

5 years ago
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.
Attachment #8340666 - Flags: feedback?(acelists) → feedback+
(Assignee)

Comment 10

5 years ago
Created attachment 8342865 [details] [diff] [review]
Patch v2.2

Thanks for the feedback.
Attachment #8340666 - Attachment is obsolete: true
Attachment #8342865 - Flags: review?(neil)
Attachment #8342865 - Flags: 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...]
(Assignee)

Comment 12

5 years ago
Created attachment 8362605 [details] [diff] [review]
Patch v2.5

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.
Attachment #8342865 - Attachment is obsolete: true
Attachment #8342865 - Flags: review?(neil)
Attachment #8362605 - Flags: review?(neil)
Created attachment 8362609 [details]
screenshot

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.
Attachment #8362609 - Attachment is obsolete: true

Comment 15

4 years ago
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?
Flags: needinfo?(neil)
(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.)
Flags: needinfo?(neil)
(Assignee)

Comment 17

4 years ago
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.
Flags: needinfo?(acelists)

Comment 18

4 years ago
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.
Flags: needinfo?(acelists)
Comment on attachment 8362605 [details] [diff] [review]
Patch v2.5

(Haven't actually tested this.)
Attachment #8362605 - Flags: review?(neil) → review+
(Assignee)

Comment 20

4 years ago
(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.
Status: NEW → ASSIGNED
I'm not privy to the plans here, but what is the way forward?
Flags: needinfo?(acelists)

Comment 22

a year ago
Created attachment 8851285 [details] [diff] [review]
Patch v2.6

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.
Attachment #8362605 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8851285 - Flags: review?(jorgk)
Attachment #8851285 - Flags: feedback+

Comment 23

a year ago
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 ;-)
Attachment #8851285 - Flags: review?(jorgk) → review+

Comment 24

a year ago
(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 25

a year ago
Created attachment 8851295 [details] [diff] [review]
Patch v2.7
Attachment #8851285 - Attachment is obsolete: true
Attachment #8851295 - Flags: review?(jorgk)

Comment 26

a year ago
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.
Attachment #8851295 - Flags: review?(jorgk) → review+

Comment 27

a year ago
Created attachment 8851332 [details] [diff] [review]
Patch v2.8

OK, that should work too.
Attachment #8851295 - Attachment is obsolete: true

Comment 28

a year ago
(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).
Component: Mail Window Front End → Backend
Keywords: checkin-needed
Product: Thunderbird → MailNews Core

Comment 29

a year ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 30

a year ago
And thanks to Thomas for poking it!!

Comment 31

a year ago
Oops, and of course thanks to Suyash!
You need to log in before you can comment on or make changes to this bug.