Closed Bug 1435034 Opened 2 years ago Closed 2 years ago

Display of default fonts is broken in preferences after Bug 1344990 and Bug 1378366

Categories

(SeaMonkey :: Preferences, defect, minor)

defect
Not set
minor

Tracking

(seamonkey2.49esr unaffected, seamonkey2.52 wontfix, seamonkey2.53 affected, seamonkey2.57esr fixed)

RESOLVED FIXED
seamonkey2.57
Tracking Status
seamonkey2.49esr --- unaffected
seamonkey2.52 --- wontfix
seamonkey2.53 --- affected
seamonkey2.57esr --- fixed

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1344990 - Needs "auto" pseudo font family to specify default font in font setting (modified labelDefaultFont property handling)

Bug 1378366 - [Preferences] No values displayed by default under Fonts & Colors on ar build (introduced labelDefaultFontUnnamed)
Attached patch Combined fix [backed out] (obsolete) — Splinter Review
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #8947580 - Flags: review?(frgrahl)
Summary: Display of default fonts is broken in preferences after bug 1344990 and bug 1378366 → Display of default fonts is broken in preferences after Bug 1344990 and Bug 1378366
Hmm there are some wbr tags in the heading. Wonder where they come from.

> show%3Cwbr%3E_bug%3Cwbr%3E.cgi?id=1344990
Comment on attachment 8947580 [details] [diff] [review]
Combined fix [backed out]

Looks good
Attachment #8947580 - Flags: review?(frgrahl) → review+
Jorg,

I think TB 58+ is missing labelDefaultFontUnnamed from Bug 1378366 too.
Flags: needinfo?(jorgk)
You mean bug 1435047? I'll land that today.
Flags: needinfo?(jorgk)
Just wanted to say that ;-)
Thanks!
Yes and does bugzilla now mess up Bug numbers in the summary? There too.
Pushed by rsx11m.pub@gmail.com:
https://hg.mozilla.org/comm-central/rev/d09c200fc3d2
Display of default fonts is broken in preferences after Bug 1344990 and Bug 1378366. r=frg
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Eh, we are missing "Target Milestone: SM 2.57" as well...
Comment on attachment 8947580 [details] [diff] [review]
Combined fix [backed out]

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

::: suite/locales/en-US/chrome/common/pref/prefutilities.properties
@@ +27,5 @@
>  
>  SoundFiles=Sounds
>  
> +# LOCALIZATION NOTE (labelDefaultFont): %S = font name
> +labelDefaultFont=Default (%S)

You can't change an existing string like this, you need a new ID.

Right now, this is showing up as an error in all languages for a different matter (strings should not have single %)
Unfortunately a string from toolkit:

https://dxr.mozilla.org/comm-central/search?q=labelDefaultFont&redirect=false

> Right now, this is showing up as an error in all languages for a different matter (strings should not have single %

Same as in browser:

> labelDefaultFont=Default (%S)
Hmm, I'm wondering if we are actually using the toolkit/mozapps/preferences/fontbuilder.js code or instead rather https://dxr.mozilla.org/comm-central/source/suite/common/pref/pref-fonts.js#51, thus overriding the toolkit handling?

But then, if the toolkit code was a NoOp, we may not have seen the error this bug was opened for?
Pushed by rsx11m.pub@gmail.com:
https://hg.mozilla.org/comm-central/rev/eecb7a8b4618
Backing out bug 1435034 changeset d09c200fc3d2 for l10n issues. rs=IanN
(In reply to Frank-Rainer Grahl (:frg) from comment #12)
> > Right now, this is showing up as an error in all languages for a different matter (strings should not have single %
> 
> Same as in browser:
> 
> > labelDefaultFont=Default (%S)

To clarify, the error is reported for localized files which still have

labelDefaultFont=Default (%font_family%)

https://l10n.mozilla.org/dashboard/compare?run=906778#issue1

I'm not talking about %S, which is a valid placeholder.

No clue why that error wasn't showing up before on dashboards.

Going back to the problem: a changed string needs a new ID
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to rsx11m from comment #13)
> Hmm, I'm wondering if we are actually using the toolkit/mozapps/preferences/fontbuilder.js code
> or instead rather https://dxr.mozilla.org/comm-central/source/suite/common/pref/pref-fonts.js#51,
> thus overriding the toolkit handling?

The latter seems to be the case, which on one hand means that we can indeed rev the property's ID, but on the other hand implies further porting efforts to adapt the changes made to fontbuilder.js in our own pref-fonts.js implementation (that includes for sure the second part of Bug 1378366).

(In reply to Francesco Lodolo [:flod] from comment #15)
> To clarify, the error is reported for localized files which still have
> 
> labelDefaultFont=Default (%font_family%)
> 
> https://l10n.mozilla.org/dashboard/compare?run=906778#issue1
> 
> I'm not talking about %S, which is a valid placeholder.

Ah, ok. So nothing wrong on our end making that change as such, just unexpected that it got marked as an error just now in the l10n versions (possibly due to a mismatch of parameters vs. en-US version?).

> Going back to the problem: a changed string needs a new ID

Yes, I'll look into that, but also need to realign our version with Toolkit's changes to fontbuilder.js made since, to ensure that everything works as expected. Hence the backout.

If that takes longer to solve, I may push an l10n-only patch in advance to get the strings in for 60ESR with a follow-up patch for any additional changes needed.

Thanks, and sorry for the screw-up.
Attachment #8947580 - Attachment description: Combined fix → Combined fix [backed out]
Attached patch More comprehensive fix (v2) (obsolete) — Splinter Review
This patch addresses the immediate port of bug 1378366 to our pref-fonts.js implementation, now showing "Default [(...)]" labels correctly. However, the lists do no longer separate the fonts of the family that we are interested in from the remaining fonts (2.49.x still do). This happens with or without this patch.

Further debugging revealed that "fonts" already contains all the fonts for gAllFonts already, thus are all considered part of the desired font type. In other words, GetFontEnumerator().EnumerateFonts(aLanguage, aFontType, {}); seems to ignore
the aFontType specification.

Given that the implementation of nsThebesFontEnumerator::EnumerateFonts() hasn't changed in ages beyond housekeeping patches, my hunch is that either bug 1344990 changed some behavior with the introduction of the "auto" pseudo font family, or it's a timing issue after bug 1375978 made font enumeration asynchronous (but we are not using the EnumerateFontsAsync function). For the latter, maybe parallel running font enumerations lead to fonts filling up completely in the same list, but I don't know enough about how it works and what the implications are.

Following the fontbuilder.js revision log, I didn't see any other changes that seem relevant.

My suggestion would be to land this and then to deal with menulist behavior in a separate bug, possibly porting bug 1375978. An alternative is obviously to switch  to Toolkit's fontbuilder.js, mirroring how Thunderbird uses it in mail/components/preferences/fonts.js (with SM-specific adaptions of course, differences appear substantial wrt language-group implementation). The latter would imply further l10n changes, given that fontbuilder.js expects a bundlePreferences definition for string properties.
Attachment #8947580 - Attachment is obsolete: true
Attachment #8948518 - Flags: review?(frgrahl)
Comment on attachment 8948518 [details] [diff] [review]
More comprehensive fix (v2)

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

::: suite/locales/en-US/chrome/common/pref/prefutilities.properties
@@ +26,5 @@
>  choosesound=Choose a sound
>  
>  SoundFiles=Sounds
>  
> +# LOCALIZATION NOTE (labelDefaultFont): %S = font name

Wrong reference (labelDefaultFont2)
grrr...
Attachment #8948518 - Attachment is obsolete: true
Attachment #8948518 - Flags: review?(frgrahl)
Attachment #8948678 - Flags: review?(frgrahl)
Was busy with the release notes and probably only have time for the review over the weekend.

I briefly looked over the font handling in browser. Isn't there a chance that we could port TB bug 1399206 instead. We are the only product not using async font handling and with all the changes we might need to do this soon anyway if the sync function goes away. The code looks very similar to me and TB is also using a xul dialog.
Flags: needinfo?(rsx11m.pub)
Sorry, I won't have anything else for you to review by this weekend (not much time).

I've had a look at the release schedule, and we have actually about 4 useful weeks left to find a better solution before the l10n freeze for 2.57 kicks in with the merge into comm-beta.

(In reply to Frank-Rainer Grahl (:frg) from comment #20)
> Isn't there a chance that we could port TB bug 1399206 instead.

I think it may be more worthwhile to try getting us on using FontBuilder, thus porting the respective snippets in display.js and fonts.js of the Thunderbird implementation (which I first need to understand before I can tell how much of a straight port that is). This will undoubtedly have l10n implications given that Toolkit expects the stringbundle at a different place. On the downside, this would tie us into whatever Firefox does, but if we decide to fork again later, at least we would be on par with TB.

I'd nevertheless like the attached patch to be reviewed as a fallback. I won't check it in right away, but want it to be ready to land before the merge if integrating FontBuilder into our pref-fonts.js fails. We can still look into making async work (and hence hopefully the font-type classification) in comm-beta, or even comm-esr60 if necessary.
Flags: needinfo?(rsx11m.pub)
Status: REOPENED → ASSIGNED
Comment on attachment 8948678 [details] [diff] [review]
Fixed localization note (v2b)

Needs a minor rebase but works fine. r+

Maybe check it in and open a followup bug?
Attachment #8948678 - Flags: review?(frgrahl) → review+
I'm still a bit undecided whether or not we should switch to using Toolkit's FontBuilder, which would have further l10n implications given that it needs an explicit stringbundle "bundlePreferences" just for the two strings changed/introduced here. On the other hand, Thunderbird's fonts.js uses only one of the functions, FontBuilder.buildFontList(), and implements the remaining functions itself. Thus, there seems to be limited value in using it.

So, if we want to keep using our own buildFontList() in pref-fonts.js, there will be a couple of more asynchronous functions to be awaited for than in bug 1399206 attachment 8907218 [details] [diff] [review], but those may be obvious from mozilla-central changeset f255ec4e8c36.

Thus, I'll land this to close this bug for now, as it solves the immediate problem, and will look into making pref-fonts.js asynchronous in a follow-up bug (i.e., without using fontbuilder.js). If that doesn't work with a reasonable effort, I'd go to try a direct adoption of Thunderbird's code.
Pushed by rsx11m.pub@gmail.com:
https://hg.mozilla.org/comm-central/rev/d116981753de
Display of default fonts is broken in preferences after Bug 1344990 and Bug 1378366 (Take II). r=frg
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Blocks: 1437393
Target Milestone: --- → Seamonkey2.57
You need to log in before you can comment on or make changes to this bug.