1465500 - Font editor: Mark unused fonts from font-family declaration

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Razvan Caliman [:rcaliman]]

The font editor UI should show the list of fonts matched from the font-family property. Unused fonts should be shown separately.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1465500 - Show list of font families declared but not used.

Review commit: https://reviewboard.mozilla.org/r/252082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252082/

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

https://reviewboard.mozilla.org/r/252082/#review258780

::: devtools/client/inspector/fonts/components/FontEditor.js:84
(Diff revision 1)
>        });
>      });
>    }
> +
> +  renderFamilesNotUsed(familiesNotUsed = []) {
> +    if (familiesNotUsed.length === 0) {

This can just be !familiesNotUsed.length

Comment 3

[Razvan Caliman [:rcaliman]]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252082/diff/1-2/

Comment 4

[Razvan Caliman [:rcaliman]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f701367a1beb4ac057008031b66ad75c7ca211

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82f89fcfaa82
Show list of font families declared but not used. r=gl

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/82f89fcfaa82

Comment 7

[Francesco Lodolo [:flod]]

# LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): This label is shown at the top
# of the list of unused font families. %S is the number of unused font families. It is
# always a positive integer larger than zero.
fontinspector.familiesNotUsedLabel=%S not used

This string requires a proper plural form (which is not 1 vs >1)
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_proper_plural_forms

Since it landed a few hours away from the final central-beta merge, and I don't want it to slip into Beta and be around for months, I'm going to ask for a backout from sheriffs. I understand it's not ideal, but I don't see any other solution.

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/5bbe54174894
Backed out changeset 82f89fcfaa82 For failing L10N by request of flod a=backout

Comment 9

[Arthur Iakab [arthur_iakab]]

Backed out changeset 82f89fcfaa82 (bug 1465500) For failing L10N by request of flod a=backout
Backout revision https://hg.mozilla.org/mozilla-central/rev/5bbe54174894e1b71aaf83006c2744bb8836816e
Failed Push:https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=c2c89b7bb8b542269ae999342835584df8a186cc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Comment 10

[Patrick Brosset <:pbro>]

Here are examples of how we use a shared PluralForm module in DevTools to deal with this: https://searchfox.org/mozilla-central/search?q=pluralform&path=devtools

Comment 11

[Razvan Caliman [:rcaliman]]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252082/diff/2-3/

Comment 12

[Francesco Lodolo [:flod]]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

https://reviewboard.mozilla.org/r/252082/#review259162

Thanks for the examples, Patrick. I always forget that DevTools forked toolkit's PluralForm.jsm

::: devtools/client/locales/en-US/font-inspector.properties:61
(Diff revisions 2 - 3)
>  # in the font editor which allows the user to change the style of the font to italic.
>  fontinspector.fontItalicLabel=Italic
>  
>  # LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): This label is shown at the top
> -# of the list of unused font families. %S is the number of unused font families. It is
> +# of the list of unused font families. #1 is the number of unused font families. It is
>  # always a positive integer larger than zero.

You don't need the last part of the comment anymore.

Could you try to keep as close as possible to the suggested comment format? i.e.

```
# LOCALIZATION NOTE (fontinspector.familiesNotUsedLabel): Semi-colon list of
# plural forms.
# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
# #1 is the number of unused font families
```

Comment 13

[Razvan Caliman [:rcaliman]]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252082/diff/3-4/

Comment 14

[Francesco Lodolo [:flod]]

Thanks, localization wise it looks good. Might be a good idea to get a second look from a devtools dev to the code changes though.

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8109dfb9cb71f71aac0372dc3ebf06976dc4866d -d f9905e10853d: rebasing 469831:8109dfb9cb71 "Bug 1465500 - Show list of font families declared but not used. r=gl" (tip)
merging devtools/client/inspector/fonts/components/FontEditor.js
merging devtools/client/inspector/fonts/fonts.js
merging devtools/client/inspector/fonts/test/browser.ini
merging devtools/client/inspector/fonts/test/browser_fontinspector.html
merging devtools/client/locales/en-US/font-inspector.properties
merging devtools/client/themes/fonts.css
warning: conflicts while merging devtools/client/inspector/fonts/test/browser_fontinspector.html! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Razvan Caliman [:rcaliman]]

Comment on attachment 8986833 [details]
Bug 1465500 - Show list of font families declared but not used.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252082/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c69831eb0e71
Show list of font families declared but not used. r=gl

Comment 18

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/c69831eb0e71