620627 - PlacesSQLQueryBuilder::SelectAsDay() is not l12y friendly

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[timeless]

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));

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 2

[timeless]

Attachment: proposal which falls back to the current behavior

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.

Comment 3

[Axel Hecht [:Pike]]

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.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the month name is localized (http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/dateFormat.properties)

Comment 5

[timeless]

Attachment: better buffer sizing

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Axel, so what I'm not sure about, can we take this in FX4?

Comment 7

[Axel Hecht [:Pike]]

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 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 499400 [details] [diff] [review]
better buffer sizing

not going to take this string change for 2.0

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 11

[timeless]

i'm not going to be working on this. sorry.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

This one should be more like we do l10n variables.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 526011 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 526011 [details] [diff] [review]
patch v1.1

that's better :)

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/projects/places/rev/cbf8fa67dd5b

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this is practically tested by existing sidebar tests.
http://hg.mozilla.org/mozilla-central/rev/cbf8fa67dd5b