Closed Bug 516354 Opened 15 years ago Closed 10 years ago

Get Mail-button should have more informative tooltip ("Get new messages for <account name>")

Categories

(Thunderbird :: Mail Window Front End, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: thomas8, Assigned: zach.x.nickell)

References

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 3 obsolete files)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090912 Shredder/3.1a1pre

Get Mail-Button on main toolbar currently has the following tooltip:
"Get new messages"

User can't tell if that's for all accounts or just the currently selected account. Ideally, the tooltip should be more informative:

"Get new messages for <currently selected account>"

Both strings exist: There's "Get new messages for" in the File menu, and we also have account names. So no L10n impact.
Severity: normal → enhancement
Whiteboard: no l10n impact
(In reply to comment #0)
> Both strings exist: There's "Get new messages for" in the File menu, and we
> also have account names. So no L10n impact.

a) That is potentially an incorrect assessment as sometimes we do need to duplicate strings in different places.

b) the has/no l10n impact is only a driver notation on blocker bugs and isn't needed for non-blocking bugs (although it may remain on some blockers if they were previously blocking and are no longer).
Whiteboard: no l10n impact
Bryan, this needs a 30 seconds UI-review from you.

The tooltip for "Get mail" button is currently "Get new mail".
This bug proposes that we be more informative here: "Get new mail for <selected_accountname>" (see mockup screenshot).
Currently, user can't tell what exactly will happen: There's no indication that it will "get mail" only for the currently selected account (and not all accounts).

(in reply to comment 1)
I understand from Mark's corrective comment that we should introduce a new string for the tooltip. We need a combined string here: "Get new messages for " + <accountname>. Mark, could you explain or point us to an explanation or sample how to construct that combined string so that it works for all kinds of languages?
Attachment #431374 - Flags: ui-review?(clarkbw)
Whiteboard: good first bug
(In reply to comment #2)
> (in reply to comment 1)
> I understand from Mark's corrective comment that we should introduce a new
> string for the tooltip. We need a combined string here: "Get new messages for "
> + <accountname>. Mark, could you explain or point us to an explanation or
> sample how to construct that combined string so that it works for all kinds of
> languages?

The basic issue is that whilst <message> + <account> name is a perfectly acceptable format for English and most other languages, its not always true. Therefore you need a different string. This is actually very easy because we do it all over the place.

https://developer.mozilla.org/en/XUL_Tutorial/Property_Files gives a background, but basically you want to define your string in a property file and have it something like

getNewMessagesTooltip=Get new messages for %S

and an appropriate localisation note saying that %S is replaced by the account name.

Like I say, we've got examples of this type of thing all around, and having the %S means that localisers can put it anywhere that's appropriate for their version of the string.
Comment on attachment 431374 [details]
Mockup Screenshot 1: Proposed Tooltip "Get new mail for <accountname>"

Even though I'd like to have the get mail button work like bug 281417 describes.  This might still be useful for the menuitems and in general the string looks fine.
Attachment #431374 - Flags: ui-review?(clarkbw) → ui-review+
Whiteboard: good first bug → [good first bug]
I may have taken this a step too far by taking into consideration the selection of multiple mail folders. 
i.e. "Get new messages for dummy@gmail.com, dummy@yahoo.com"
Attachment #8358289 - Flags: review?(bwinton)
Comment on attachment 8358289 [details] [diff] [review]
Adds function to generate dynamic tooltip for get mail button

Review of attachment 8358289 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you very much for this patch, Zach!

I'm mostly happy with it, but I've got a couple of things I'ld like you to change before we check it in.
(I'm marking it r-, so that I can take a look at your fixes, not because I'm particularly unhappy with it.)

One more note: I'm flying to Paris tonight for a week, so I may not be quite as quick to review your follow-up patch.
Or, looking at it another way, you have a week to make the changes before I get back.  ;)

Thanks,
Blake.

::: mail/base/content/mailWindowOverlay.js
@@ +724,5 @@
> +    usernames.push(folders[i].server.username);
> +  }
> +  var uniqueUsernames = usernames.filter(function(elem, pos) {
> +    return usernames.indexOf(elem) == pos;
> +  });

One of the nice things about writing Thunderbird front-end code is that we know the exact version of the JavaScript engine we're running in, and so can use some nice new features to make our lives better.

In this case, I think you should use "let" for the array index to scope it to the array, and use a Set instead of creating an array and filtering it.  :)

@@ +727,5 @@
> +    return usernames.indexOf(elem) == pos;
> +  });
> +  for(var i = 0; i < uniqueUsernames.length; i++){
> +    if (i > 0) {
> +      tooltipUsernames += ", " + uniqueUsernames[i];

I wonder a little bit about whether the comma is used to separate lists of things in other languages…

http://stackoverflow.com/questions/7195993/localization-and-lists-of-decimal-numbers suggests perhaps it isn't, and browser/locales/en-US/chrome/browser/syncQuota.properties has a quota.list.separator property, which would indicate the same thing.

I think we want to follow the syncQuota's lead here.

::: mail/base/content/mailWindowOverlay.xul
@@ +3018,5 @@
>      <toolbarbutton id="button-getmsg"
>                     type="menu-button"
>                     class="toolbarbutton-1"
>                     label="&getMsgButton1.label;"
> +                   onmouseover="SetGetMsgButtonTooltip(this)"

This seems like a fairly expensive function to run on every mouseover.
What if we ran it on folder selection changed instead?
Attachment #8358289 - Flags: review?(bwinton) → review-
I believe this should be better:)
Attachment #8358289 - Attachment is obsolete: true
Attachment #8362046 - Flags: review?(bwinton)
Comment on attachment 8362046 [details] [diff] [review]
Adds function to generate dynamic tooltip for get mail button v2

Review of attachment 8362046 [details] [diff] [review]:
-----------------------------------------------------------------

This was certainly better, but I think we can make it better still…
(This will likely be the last set of changes I ask for, but I'm marking it ui-r- just to give it one more pass.  :)

::: mail/base/content/mailWindowOverlay.js
@@ +712,5 @@
>      menuItem.setAttribute('label', customLabel);
>  }
>  
> +function SetGetMsgButtonTooltip()
> +{ 

Nit: There's a single extra space at the end of this line.

@@ +720,5 @@
> +  var msgButton = document.getElementById("button-getmsg");
> +  var tooltip = document.getElementById("bundle_messenger").getString("getMsgButtonTooltip");
> +  var listSeperator = document.getElementById("bundle_messenger").getString("getMsgButtonTooltip.listSeparator");
> +  var tooltipUsernames = "";
> +  let usernames = new Set();

It seems odd to use "let" here, and "var" everywhere else.  Let's change this to "var" too.

@@ +722,5 @@
> +  var listSeperator = document.getElementById("bundle_messenger").getString("getMsgButtonTooltip.listSeparator");
> +  var tooltipUsernames = "";
> +  let usernames = new Set();
> +  for (let i = 0; i < folders.length; i++) {
> +    if (!usernames.has(folders[i].server.username))

If we're using a Set, we don't need to do this check.  We can just add the username, and let the Set worry about the duplications.  :)

@@ +724,5 @@
> +  let usernames = new Set();
> +  for (let i = 0; i < folders.length; i++) {
> +    if (!usernames.has(folders[i].server.username))
> +      usernames.add(folders[i].server.username);
> +  }

And, come to think of it, it might be easier to write that whole series of lines as:
  var usernames = new Set([v.server.username for (v in folders)]);
or something along those lines…

@@ +730,5 @@
> +    let usrarr = [v for (v of usernames)];
> +    tooltipUsernames += value;
> +    if (value != usrarr[usernames.size-1])
> +      tooltipUsernames += listSeperator;
> +  });

I'm not sure what this code is trying to do, but it seems like it's doing a lot of work.  (For instance, it's re-creating the usrarr each time through the loop…)  Could we use something like:
  var usernamesArray = [username for (username of usernames)];
  var tooltipUsernames = usernamesArray.join(listSeparator);
instead?

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +136,5 @@
>  linesColumnTooltip=Click to sort by lines
>  linesColumnHeader=Lines
>  
> +# getMsgButton tooltip
> +getMsgButtonTooltip=Get new messages for %S

I think we need a LOCALIZATION NOTE here, for the translators, to explain what %S is going to be.
Attachment #8362046 - Flags: review?(bwinton) → review-
Thanks for all these reviews Blake!
If those last patches were any indication, it's been a little while since I'd last used javascript;)
Attachment #8362046 - Attachment is obsolete: true
Attachment #8367617 - Flags: review?(bwinton)
Comment on attachment 8367617 [details] [diff] [review]
8362046: Adds function to generate dynamic tooltip for get mail button v3

Review of attachment 8367617 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  Thanks for the patch!  :)
Attachment #8367617 - Flags: review?(bwinton) → review+
Comment on attachment 8367617 [details] [diff] [review]
8362046: Adds function to generate dynamic tooltip for get mail button v3

Review of attachment 8367617 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Zach Nickell from comment #9)
> Created attachment 8367617 [details] [diff] [review]
> 8362046: Adds function to generate dynamic tooltip for get mail button v3
> Thanks for all these reviews Blake!

+1

Zach, nice to meet you here and thanks for writing up the patch for my RFE :)

f+ = me and ready for landing (checkin-needed) with the following nit fixed (this has two ui-reviews from Bryan and Blake)

::: mail/base/content/mailWindowOverlay.js
@@ +718,5 @@
> +  var defaultAccountRootFolder = GetDefaultAccountRootFolder();
> +  var folders = (selectedFolders.length) ? selectedFolders : [defaultAccountRootFolder];
> +  var msgButton = document.getElementById("button-getmsg");
> +  var tooltip = document.getElementById("bundle_messenger").getString("getMsgButtonTooltip");
> +  var listSeperator = document.getElementById("bundle_messenger").getString("getMsgButtonTooltip.listSeparator");

spelling: listSeperator should be listSeparator (2 occurences)
Attachment #8367617 - Flags: feedback+
(In reply to Thomas D. (away till 31st January) from comment #11)
> Zach, nice to meet you here and thanks for writing up the patch for my RFE :)
Nice to meet you here as well :)

> spelling: listSeperator should be listSeparator (2 occurences)
Oh geeze, I could've sworn I'd fixed that.
Do I need to submit a patch with those corrections?
Yes please.
Comment on attachment 8368787 [details] [diff] [review]
Adds function to generate dynamic tooltip for get mail button v3.1 (spelling fix)

Ready for landing:

r+ from :bwinton (comment 10), which also implies:
ui-r+ from :bwinton as Blake monitored and advised on the iterations of this patch since comment 6 (and previous ui-r+ from Bryan, comment 4).
Attachment #8368787 - Flags: ui-review+
Attachment #8368787 - Flags: review+
Attachment #8368787 - Flags: feedback+
(In reply to Thomas D. (away till 31st January) from comment #15)
> Comment on attachment 8368787 [details] [diff] [review]
> Adds function to generate dynamic tooltip for get mail button v3.1 (spelling
> fix)
> 
> Ready for landing:
> 
> r=bwinton (comment 10), which also implies:
> ui-r=bwinton,clarkbw as Blake monitored and advised on the iterations of this
> patch since comment 6, and previous ui-r+ from Bryan, comment 4.
Keywords: checkin-needed
Attachment #8367617 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/1a226670cfeb
Assignee: nobody → zach.x.nickell
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
OK, but see bug 281417, where people actually want to turn this over and make the button mean "for all accounts".
Depends on: 967583
You need to log in before you can comment on or make changes to this bug.