Closed Bug 620627 Opened 10 years ago Closed 10 years ago

PlacesSQLQueryBuilder::SelectAsDay() is not l12y friendly

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
status2.0 --- wontfix

People

(Reporter: timeless, Assigned: mak)

References

Details

(Keywords: l12y, Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(1 file, 2 obsolete files)

3523 PlacesSQLQueryBuilder::SelectAsDay()

3563     // These are used to query if the container should be visible.
3565     switch(i) {
3626        default:
3658         history->GetMonthName(tm.tm_month+1, dateName);

this is not localization friendly, it's perfectly reasonable for some locale to want "2000 January" instead of "January 2000" (it might even want commas or hyphens or something more exotic):
3660         // If the container is for a past year, add the year as suffix.
3661         if (tm.tm_year < currentYear)
3662           dateName.Append(nsPrintfCString(" %d", tm.tm_year));
I'm pretty sure this was discussed, but probably never fixed because it's a edge case and we were evaluating a history views redesign.
So, this code should result in nothing changing.

Locales which don't provide this string should trigger the fallback behavior which is the current l12y unfriendly pasting. Anyone who happens to include the string would get whatever format the string provides.
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #499059 - Flags: review?(mak77)
Attachment #499059 - Flags: feedback?(l10n)
Comment on attachment 499059 [details] [diff] [review]
proposal which falls back to the current behavior

No idea why we'd need to fallback for missing strings here, we've got l10n-merge for that.

Hardcoding the length to 32 sounds arbitrary, too, I'd rather check the length of the format string and add some padding for year and month length. Where do we get the month name from, though? If that stays English, an English date order might be less confusing than the other way around.
Attachment #499059 - Flags: feedback?(l10n) → feedback-
Attached patch better buffer sizing (obsolete) — Splinter Review
Attachment #499059 - Attachment is obsolete: true
Attachment #499400 - Flags: review?(mak77)
Attachment #499400 - Flags: approval2.0?
Attachment #499059 - Flags: review?(mak77)
Axel, so what I'm not sure about, can we take this in FX4?
I don't think we should take this for fx4, it doesn't seem to be a regression and we're string frozen.

PS: I still don't like the fallback code for a missing string.
Comment on attachment 499400 [details] [diff] [review]
better buffer sizing

not going to take this string change for 2.0
Attachment #499400 - Flags: approval2.0? → approval2.0-
Comment on attachment 499400 [details] [diff] [review]
better buffer sizing

At this point, please make a proper non-fallback patch for .next version

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+  nsCString monthYearFormat;

why not a autostring?

>@@ -3431,8 +3433,17 @@ PlacesSQLQueryBuilder::SelectAsDay()
>         history->GetMonthName(tm.tm_month+1, dateName);
> 
>         // If the container is for a past year, add the year as suffix.

no more a suffix, not always at least, fix comment to just say ", add the year to the title."

>+        if (tm.tm_year < currentYear) {
>+          if (monthYearFormat.IsEmpty()) {
>+            history->GetStringFromName(NS_LITERAL_STRING("finduri-MonthYear").get(), monthYearFormat);

Why not just doing this when you declare monthYearFormat?

>+            if (monthYearFormat.EqualsLiteral("finduri-MonthYear")) {
>+              monthYearFormat.AssignLiteral("%s %d");

drop the fallback

>+            }
>+          }
>+          dateName =
>+              nsPrintfCString(monthYearFormat.Length() + dateName.Length() + 10,

why 10?
Attachment #499400 - Flags: review?(mak77) → review-
status2.0: --- → wontfix
Whiteboard: [places-next-wanted]
Duplicate of this bug: 486825
i'm not going to be working on this. sorry.
Assignee: timeless → mak77
Status: ASSIGNED → NEW
Attached patch patch v1.1Splinter Review
This one should be more like we do l10n variables.
Attachment #499400 - Attachment is obsolete: true
Attachment #526011 - Flags: review?(sdwilsh)
Attachment #526011 - Flags: feedback?(gandalf)
Comment on attachment 526011 [details] [diff] [review]
patch v1.1

r=sdwilsh
Attachment #526011 - Flags: review?(sdwilsh) → review+
Comment on attachment 526011 [details] [diff] [review]
patch v1.1

that's better :)
Attachment #526011 - Flags: feedback?(gandalf) → feedback+
http://hg.mozilla.org/projects/places/rev/cbf8fa67dd5b
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
this is practically tested by existing sidebar tests.
http://hg.mozilla.org/mozilla-central/rev/cbf8fa67dd5b
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.