Closed Bug 633730 Opened 9 years ago Closed 7 years ago

Port |Bug 630484 - Properly support plural forms in advance search dialog status message| to SeaMonkey

Categories

(SeaMonkey :: MailNews: Message Display, defect, minor)

defect
Not set
minor

Tracking

(seamonkey2.13 wontfix, seamonkey2.14 affected)

RESOLVED FIXED
seamonkey2.14
Tracking Status
seamonkey2.13 --- wontfix
seamonkey2.14 --- affected

People

(Reporter: sgautherie, Assigned: tonymec)

References

(Blocks 1 open bug, )

Details

(Keywords: verifyme, Whiteboard: l10n[good first bug][mentor=IanN][lang=js])

Attachments

(1 file, 4 obsolete files)

No description provided.
Component: Search → MailNews: Message Display
QA Contact: search → message-display
Whiteboard: [good first bug] [TB2SM] → [good first bug][mentor=??][lang=js] [TB2SM]
Blocks: TB2SM
Whiteboard: [good first bug][mentor=??][lang=js] [TB2SM] → [good first bug][mentor=??][lang=js]
Robert, would you like to mentor this bug?
Whiteboard: [good first bug][mentor=??][lang=js] → [good first bug][mentor=ask in #seamonkey][lang=js]
(In reply to Philip Chee from comment #1)
> Robert, would you like to mentor this bug?

Not really, I rarely have any time to care about any SeaMonkey topics, and people are leaving me enough work with untested changes in modules I own that all SeaMonkey time I have this year is probably booked out already right now.
Whiteboard: [good first bug][mentor=ask in #seamonkey][lang=js] → [good first bug][mentor=IanN][lang=js]
Attached patch patch v0 (obsolete) — Splinter Review
Assignee: nobody → antoine.mechelynck
Status: NEW → ASSIGNED
Attachment #647887 - Flags: review?(iann_bugzilla)
Comment on attachment 647887 [details] [diff] [review]
patch v0

Looks good so far, but you also have to change:
http://mxr.mozilla.org/comm-central/source/suite/mailnews/searchBar.js#19
Attachment #647887 - Flags: review?(iann_bugzilla) → review-
Attached patch patch v1 (obsolete) — Splinter Review
In reply to comment #4:
Ah, OK, I'd missed that. I ought to have run an mxr search for "searchFailureMessage" (now I did, and nothing else came up).
Attachment #647887 - Attachment is obsolete: true
Attachment #648988 - Flags: review?(iann_bugzilla)
Attached patch patch v1.1 (obsolete) — Splinter Review
oops, wrong indent in "else" branch of if
Attachment #648988 - Attachment is obsolete: true
Attachment #648988 - Flags: review?(iann_bugzilla)
Attachment #648991 - Flags: review?(iann_bugzilla)
Comment on attachment 648991 [details] [diff] [review]
patch v1.1

>+++ b/suite/mailnews/searchBar.js

> function SetQSStatusText(aNumHits)
> {
>   var statusMsg;
>   // if there are no hits, it means no matches were found in the search.
>+  if (aNumHits == 0) {
>+    statusMsg = gSearchBundle.getString("noMatchesFound");
>+  } else {
>+    statusMsg = PluralForm.get(aNumHits,
>+                               gSearchBundle.getString("matchesFound");
Missing ) after argument list.
r=me with that fixed.
Attachment #648991 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v1.2 r=IanN (obsolete) — Splinter Review
Duh. I really should have pasted the change by mouse instead of by eyeball.
Attachment #648991 - Attachment is obsolete: true
Attachment #649085 - Flags: review+
oops, it's a mailnews patch.
Keywords: checkin-needed
Comment on attachment 649085 [details] [diff] [review]
patch v1.2 r=IanN

Just one nit:

>+  if (aNumHits == 0) {
>+    statusMsg = gSearchBundle.getString("noMatchesFound");
>+  } else {
>+    statusMsg = PluralForm.get(aNumHits,
>+                               gSearchBundle.getString("matchesFound"));
>+    statusMsg = statusMsg.replace("#1", aNumHits);
>   }

Braces should go onto their own line, see <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle#Default_bracing_style_is_.22curly_braces_go_on_their_own_line.22>.

moa=me with that fixed throughout the whole patch.
Attachment #649085 - Flags: superreview?(mnyromyr) → superreview+
Ah, OK.
…and I also saw the next section, "braces on both sides of an else, or on neither", so the braces around the single-statement "true" paths must stay.

Hg qrefresh removed and added the else rather than the { but what can I do? Thats how it is. :-/
Attachment #649085 - Attachment is obsolete: true
Attachment #649140 - Flags: superreview+
Attachment #649140 - Flags: review+
https://hg.mozilla.org/comm-central/rev/f31b3e24565f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.14
Checking this fix correctly requires testing a localized version in a language with more than two "plural forms". The UI languages currently built every night for Sm trunk are: be, de, en-US, es-AR, hu, it, gl, lt, nl, ru, sk, uk, zh-CN, zh-TW but of course the localizers will need some time to add the new strings, which are:
- "No matches found" (existed already, but the string name changed)
- "#n match(es) found" (new localizable string, depending on the value of the integer n >= 1).
The second string has in practice two values (as shown) for English; in other languages it may be fewer (Chinese=1), the same (Western European), or more.
Keywords: verifyme
Whiteboard: [good first bug][mentor=IanN][lang=js] → l10n
Whiteboard: l10n → l10n[good first bug][mentor=IanN][lang=js]
Comment on attachment 649140 [details] [diff] [review]
patch v1.3 r=IanN moa=Mnyromyr

Pike: How do you think this fix should best be VERIFIED?
Attachment #649140 - Flags: feedback?(l10n)
Comment on attachment 649140 [details] [diff] [review]
patch v1.3 r=IanN moa=Mnyromyr

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

Sorry, no good idea. Not sure if you could hook up different plural rules by executing a line like http://mxr.mozilla.org/mozilla-central/source/intl/locale/src/PluralForm.jsm#101 with an explicit number for the plural form and test for that.
Attachment #649140 - Flags: feedback?(l10n)
You need to log in before you can comment on or make changes to this bug.