Closed Bug 1465500 Opened 2 years ago Closed 2 years ago

Font editor: Mark unused fonts from font-family declaration

Categories

(DevTools :: Inspector: Fonts, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(1 file)

The font editor UI should show the list of fonts matched from the font-family property. Unused fonts should be shown separately.
Blocks: 1465424
No longer blocks: 1465424
Product: Firefox → DevTools
Summary: Font editor: Mark used and unused fonts from font-family declaration → Font editor: Mark unused fonts from font-family declaration
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
Attachment #8986833 - Flags: review?(gl) → review+
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82f89fcfaa82
Show list of font families declared but not used. r=gl
https://hg.mozilla.org/mozilla-central/rev/82f89fcfaa82
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
# 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.
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/5bbe54174894
Backed out changeset 82f89fcfaa82 For failing L10N by request of flod a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 62 → ---
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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 62 → ---
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 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
```
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.
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)
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c69831eb0e71
Show list of font families declared but not used. r=gl
https://hg.mozilla.org/mozilla-central/rev/c69831eb0e71
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.