Closed Bug 588759 Opened 9 years ago Closed 5 years ago

Make sure status bar messages have proper punctuation

Categories

(Thunderbird :: Message Reader UI, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 38.0

People

(Reporter: andreasn, Assigned: mkmelin, Mentored)

Details

(Whiteboard: [good first bug][school project])

Attachments

(1 file, 3 obsolete files)

Spinning off bug 79470 as an interesting discussion started there if we should use punctuation after the messages or not.
https://bugzilla.mozilla.org/show_bug.cgi?id=579470#c6
OS: Linux → All
According to Matthew Paul Thomas, when I spoke to him on IRC, we should not use full stops in those situations.

<mpt_> andreasn, when I worked on status bar messages for Mozilla (which were inherited by Firefox), I specified that temporary or progress messages should end in ellipsis (e.g. "Waiting for bugzilla.mozilla.org…"), while others should not end in any punctuation (e.g. "Done")
Severity: normal → trivial
Whiteboard: [good first bug]
One sample - http://hg.mozilla.org/comm-central/annotate/6c605636afe1/mail/locales/en-US/chrome/messenger/imapMsgs.properties#l217

(though that file is i believe being changed away from number based strings)

Probably a lot of investigation for someone taking this on to find and check which things are statues... Also note you have to change the localization key when changing the value, otherwise localizers can't keep up.
Hardware: x86_64 → All
Whiteboard: [good first bug] → [good first bug][mentor=mkmelin]
Hey all- New to the community, trying to get in on development. I figure this bug is fairly straightforward, something to get me familiar with some of the directory structure. So... I'd be happy to take this on.
Quick question: if a message has multiple sentences, should I still take off the final period? Personally, it feels a little unbalanced in that case.
Yes, for multiple sentences there should probably always be a final period.
Mentor: mkmelin+mozilla
Whiteboard: [good first bug][mentor=mkmelin] → [good first bug]
Assignee: nobody → abaptist
Whiteboard: [good first bug] → [good first bug][school project]
I'm having a little trouble finding where the text for the status bar is in the code. Could someone tell me where I should look?
Sorry to double post. I had meant to ask if the sample given by Magnus Melin was the only portion that needed editing. I would like to take the time to consult with someone over each line and it would be better if I gathered as many of the messages as possible beforehand.
I had some difficulty learning to use Mercurial and the other tools. Because of that, it is likely that the patch itself is formatted incorrectly. Handle with caution.
The patch format looks good, but preferably use an 8 line context for easier reviews
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F 

... 

[diff]
git = 1
showfunc = 1
unified = 8

Also, the patch comment is usually the bug summary.

After you fix those minor issues, please submit the patch for review by setting the "review?" flag to suitable reviewers.
(Me, maybe iann_bugzilla@blueyonder.co.uk, and ui-review from josiah@programmer.net)

https://developer.mozilla.org/docs/En/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Comment on attachment 8491885 [details] [diff] [review]
statuses.patch changes the formatting of status messages and their localization keys

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

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +202,4 @@
>  # Place the word %3$S in your translation where the name of the destination folder should appear.
>  # Place the word %1$S where the currently copying message should appear.
>  # Place the word %2$S where the total number of messages should appear.
> +imapCopyingMessageOf2=Copying Message %1$S of %2$S to %3$S…

we should make this Message lower cased while we're touching this
Some status messages in imapMsgs.properties under mail used incorrect punctuation.
Attachment #8492561 - Flags: ui-review?(josiah)
Attachment #8492561 - Flags: review?(mkmelin+mozilla)
Attachment #8492561 - Flags: review?(iann_bugzilla)
Comment on attachment 8492561 [details] [diff] [review]
status.diff changes status message formatting and localization keys


>-# LOCALIZATION NOTE (imapCopyingMessageOf): Do not translate the word "%S" below.
>+# LOCALIZATION NOTE (imapCopyingMessageOf2): Do not translate the word "%S" below.
> # Place the word %3$S in your translation where the name of the destination folder should appear.
> # Place the word %1$S where the currently copying message should appear.
> # Place the word %2$S where the total number of messages should appear.
>-imapCopyingMessageOf=Copying Message %1$S of %2$S to %3$S
>+imapCopyingMessageOf2=Copying Message %1$S of %2$S to %3$S…

You missed changing Message to message here.
Yes, it shows up twice and I only noticed the first. Is there a way to replace or edit an attachment or do I just have to post it again?(In reply to Ian Neal from comment #12)
> Comment on attachment 8492561 [details] [diff] [review]
> status.diff changes status message formatting and localization keys
> 
> 
> >-# LOCALIZATION NOTE (imapCopyingMessageOf): Do not translate the word "%S" below.
> >+# LOCALIZATION NOTE (imapCopyingMessageOf2): Do not translate the word "%S" below.
> > # Place the word %3$S in your translation where the name of the destination folder should appear.
> > # Place the word %1$S where the currently copying message should appear.
> > # Place the word %2$S where the total number of messages should appear.
> >-imapCopyingMessageOf=Copying Message %1$S of %2$S to %3$S
> >+imapCopyingMessageOf2=Copying Message %1$S of %2$S to %3$S…
> 
> You missed changing Message to message here.
(In reply to Andre Baptiste from comment #13)
> Yes, it shows up twice and I only noticed the first. Is there a way to
> replace or edit an attachment or do I just have to post it again?(In reply
> to Ian Neal from comment #12)
> > Comment on attachment 8492561 [details] [diff] [review]
> > status.diff changes status message formatting and localization keys
> > 
> > 
> > >-# LOCALIZATION NOTE (imapCopyingMessageOf): Do not translate the word "%S" below.
> > >+# LOCALIZATION NOTE (imapCopyingMessageOf2): Do not translate the word "%S" below.
> > > # Place the word %3$S in your translation where the name of the destination folder should appear.
> > > # Place the word %1$S where the currently copying message should appear.
> > > # Place the word %2$S where the total number of messages should appear.
> > >-imapCopyingMessageOf=Copying Message %1$S of %2$S to %3$S
> > >+imapCopyingMessageOf2=Copying Message %1$S of %2$S to %3$S…
> > 
> > You missed changing Message to message here.

You can't modify the bugzilla attachment, you will have to upload a new one. Now if you understand HG decently well, you need not change your source code, but instead can modify your existing patch file.
Some status messages in imapMsgs.properties under mail used incorrect end punctuation and capitalization.
Attachment #8491885 - Attachment is obsolete: true
Attachment #8492561 - Attachment is obsolete: true
Attachment #8492561 - Flags: ui-review?(josiah)
Attachment #8492561 - Flags: review?(mkmelin+mozilla)
Attachment #8492561 - Flags: review?(iann_bugzilla)
Attachment #8492822 - Flags: ui-review?(josiah)
Attachment #8492822 - Flags: review?(mkmelin+mozilla)
Attachment #8492822 - Flags: review?(iann_bugzilla)
Comment on attachment 8492822 [details] [diff] [review]
status.diff changes the formatting of status messages and their localization keys

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

It appears all the ellipsis are now corrupted. Please ensure everything is UTF-8
Attachment #8492822 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 8492822 [details] [diff] [review]
status.diff changes the formatting of status messages and their localization keys

>+++ b/mail/locales/en-US/chrome/messenger/imapMsgs.properties

>+# LOCALIZATION NOTE (imapReceivingMessageHeaders2): Do not translate the word "%1$S", "%2$lu" or "%3$lu" below.
> # Place the word %1$S in your translation where the name of the server should appear.
> # Place the word %2$lu where the number of the header currently being downloaded should appear.
> # Place the word %3$lu where the number of headers should appear.
These comments no longer match what is below. They appear to be correct in suite/ equivalent though.
>+imapReceivingMessageHeaders2=%S Downloading message header %lu of %lu…


>+# LOCALIZATION NOTE (imapReceivingMessageFlags2): Do not translate the word "%1$S", "%2$lu" or "%3$lu" below.
> # Place the word %1$S in your translation where the name of the server should appear.
> # Place the word %2$lu where the number of the flag currently being downloaded should appear.
> # Place the word %3$lu where the number of flags should appear.
These comments no longer match what is below. They appear to be correct in suite/ equivalent though.
>+imapReceivingMessageFlags2=%S Downloading message flag %lu of %lu…
Attachment #8492822 - Flags: review?(iann_bugzilla) → review-
Comment on attachment 8492822 [details] [diff] [review]
status.diff changes the formatting of status messages and their localization keys

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

I don't know why you're adding '2' to everything, and the fact that this is not in UTF-8 means ui-r- for now.
Attachment #8492822 - Flags: ui-review?(josiah) → ui-review-
Andre: any update?
Flags: needinfo?(abaptist)
(In reply to Magnus Melin from comment #19)
> Andre: any update?

I'm afraid not. I've gotten caught up in some school project so I don't know when I'll come back to this. I'll try again once I have more time but until then I should probably be taken off.
Flags: needinfo?(abaptist)
Attached patch 588759.patchSplinter Review
Updated the patch, so we can hopefully get this landed. r=me for the mail/ part

Josiah: to answer your question in comment 18. We have to update the localization key, so localizers notice the change.
Assignee: abaptist → mkmelin+mozilla
Attachment #8543184 - Flags: ui-review?(josiah)
Attachment #8543184 - Flags: review+
Attachment #8492822 - Attachment is obsolete: true
Attachment #8543184 - Flags: review?(iann_bugzilla)
Comment on attachment 8543184 [details] [diff] [review]
588759.patch

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

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +77,5 @@
>  
> +# LOCALIZATION NOTE (imapReceivingMessageHeaders2): Do not translate the word "%S" or "%lu" below.
> +# Place the word %S in your translation where the name of the server should appear.
> +# Place the word %lu where the number of headers should appear.
> +imapReceivingMessageHeaders2=%S Downloading message header %lu of %lu…

I think the encoding got messed up (Happens here and many other places).

::: suite/locales/en-US/chrome/mailnews/imapMsgs.properties
@@ +84,3 @@
>  # Place the word %S in your translation where the name of the server should appear.
>  # Place the word %lu where the number of headers should appear.
> +imapReceivingMessageHeaders2=%S Downloading message header %lu of %lu…

I think the encoding got messed up (Happens here and many other places).
It appears splinter mangles the ellipsis. If you look at it raw, https://bug588759.bugzilla.mozilla.org/attachment.cgi?id=8543184 its correctly UTF-8
Comment on attachment 8543184 [details] [diff] [review]
588759.patch

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

Ah, alright. Looks fine then.
Attachment #8543184 - Flags: ui-review?(josiah) → ui-review+
Attachment #8543184 - Flags: review?(iann_bugzilla) → review+
https://hg.mozilla.org/comm-central/rev/d2889360a981 -> FIXED

Apparently there's a bug in hg that made the UTF-8 messed up for display in the patch. The problem seems to be that an ellipsis is on the hunk marker row (@@ -102,20 +100,20 @@ ) and THAT is wrongly encoded, though it's correct in the actual file.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
You need to log in before you can comment on or make changes to this bug.