-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Noto Sans Arabic: Version 2.012 added #6859
Conversation
c81407f
to
3920071
Compare
FontBakery reportfontbakery version: 0.10.0 [1] Family checksℹ INFO: Check axis ordering on the STAT table. (com.google.fonts/check/STAT/axis_order)
[16] NotoSansArabic[wdth,wght].ttf🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent. (com.google.fonts/check/family/win_ascent_and_descent)
⚠ WARN: Check for codepoints not covered by METADATA subsets. (com.google.fonts/check/metadata/unreachable_subsetting)
Use -F or --full-lists to disable shortening of long lists. Or you can add the above codepoints to one of the subsets supported by the font: ⚠ WARN: Glyphs are similiar to Google Fonts version? (com.google.fonts/check/production_glyphs_similarity)
Use -F or --full-lists to disable shortening of long lists. ⚠ WARN: Is there kerning info for non-ligated sequences? (com.google.fonts/check/kerning_for_non_ligated_sequences)
⚠ WARN: Ensure fonts have ScriptLangTags declared on the 'meta' table. (com.google.fonts/check/meta/script_lang_tags)
⚠ WARN: Glyph names are all valid? (com.google.fonts/check/valid_glyphnames)
⚠ WARN: Check font contains no unreachable glyphs (com.google.fonts/check/unreachable_glyphs)
⚠ WARN: Check math signs have the same width. (com.google.fonts/check/math_signs_width)
Width = 322: ⚠ WARN: Ensure soft_dotted characters lose their dot when combined with marks that replace the dot. (com.google.fonts/check/soft_dotted)
The dot of soft dotted characters should disappear in other cases, for example: į̆ į̇ į̈ į̊ į̋ į̒ į̦̀ į̦́ į̦̂ į̦̃ į̦̄ į̦̆ į̦̇ į̦̈ į̦̊ į̦̋ į̦̌ į̦̒ į̧̀ į̧́ Your font fully covers the following languages that require the soft-dotted feature: Lithuanian (Latn, 2,357,094 speakers). Your font does not cover the following languages that require the soft-dotted feature: Koonzime (Latn, 40,000 speakers), Ma’di (Latn, 584,000 speakers), Basaa (Latn, 332,940 speakers), Nateni (Latn, 100,000 speakers), Lugbara (Latn, 2,200,000 speakers), Aghem (Latn, 38,843 speakers), Kom (Latn, 360,685 speakers), Ukrainian (Cyrl, 29,273,587 speakers), Dutch (Latn, 31,709,104 speakers), Ejagham (Latn, 120,000 speakers), Dan (Latn, 1,099,244 speakers), Avokaya (Latn, 100,000 speakers), Igbo (Latn, 27,823,640 speakers), Belarusian (Cyrl, 10,064,517 speakers), Navajo (Latn, 166,319 speakers), Ebira (Latn, 2,200,000 speakers). [code: soft-dotted] ℹ INFO: Show hinting filesize impact. (com.google.fonts/check/hinting_impact)
ℹ INFO: Font has old ttfautohint applied? (com.google.fonts/check/old_ttfautohint)
ℹ INFO: EPAR table present in font? (com.google.fonts/check/epar)
ℹ INFO: Is the Grid-fitting and Scan-conversion Procedure ('gasp') table set to optimize rendering? (com.google.fonts/check/gasp)
PPM <= 65535: ℹ INFO: Check for font-v versioning. (com.google.fonts/check/fontv)
ℹ INFO: Font contains all required tables? (com.google.fonts/check/required_tables)
ℹ INFO: List all superfamily filepaths (com.google.fonts/check/superfamily/list)
Summary
Note: The following loglevels were omitted in this report:
|
This release:
|
Unicode 15 (there were 3 new Arabic code points added in Unicode 15 that are npw supported as well) |
@simoncozens There is one FontBakery vertical metrics fail, is there a good reason to ignore this? Or should it be fixed and the PR updated before merging? 🔥 FAIL: Checking OS/2 usWinAscent & usWinDescent. (com.google.fonts/check/family/win_ascent_and_descent) I looked around in previous PRs, but couldn't find any prior discussion of this. |
One thing to note for anyone else reviewing this, the change to |
@simoncozens the Heh I have basic Arabic reading skills, and I would find this strange and would expect the circular form without the continuation into the middle loop when typing I am not an Arabic expert, but I have looked at and used a lot of Arabic fonts and this seems unusual to me: I feel like this is a big enough change it needs an explanation before merging. Edit: there is a PR fixing this open : notofonts/arabic#207 |
No, that PR fixes something different - FEE9, rather than 0647. Khaled's explanation for choice to use a double-eyed form for ہ is here:
However: the Unicode Standard and code charts use the single-eyed form as "Xn" (isolated): Which means we seem to have got ahead of Unicode here. @khaledhosny, I think since Noto is an implementation of Unicode, we may need to back out this change. |
The Unicode chart uses this form: and the Arabic chapter has this text (though the text is rather misleading, as it is not the initial form but a form very close to it and the initial form is used as a fallback by users): So there is awareness that isolated heh takes a different form I don’t know why this was not reflected in the table, but this probably would be a 4th column, since heh has 4 forms not 3. |
I am in favor of using the double-eyed form isolated heh after looking into this more. It is a legibility improvement, it is rendered this way on the Apple keyboards, and I trust Khaled's expertise. This is not a merge blocker from me, and I personally think it is an improvement. Edit: @simoncozens @khaledhosny Just an idea, but what if the stroke did not extend to the left as a compromise between the two forms? I think maybe I have seen this in manuscripts and could look for examples: Note that this is already happening with the Nastaliq Heh in Table 9-9 above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost all the issues I opened in the upstream git repo have been closed by this PR and the one remaining one does not need to be resolved anytime soon: notofonts/arabic#212
There is a commit to the upstream repository fixing the one remaining FontBakery fail: Update OS/2 usWinAscent for all fonts. @simoncozens can we rebuild the fonts and update this PR with all the new changes? After that this should be ok to merge. Thanks!
* Noto Sans Arabic Version 2.012 taken from the upstream repo https://github.com/notofonts/arabic.git at commit https://github.com/notofonts/arabic/commit/.
Updated Noto Sans Arabic: Version 2.012 addede5ee313: [gftools-packager] Noto Sans Arabic: Version 2.012 added
|
3920071
to
e5ee313
Compare
FontBakery reportfontbakery version: 0.10.3 [1] Experimental checks🔥 FAIL: Shapes languages in all GF glyphsets. (com.google.fonts/check/glyphsets/shape_languages)
[1] Family checksℹ INFO: Check axis ordering on the STAT table. (com.google.fonts/check/STAT/axis_order)
[16] NotoSansArabic[wdth,wght].ttf🔥 FAIL: Check that legacy accents aren't used in composite glyphs. (derived from com.google.fonts/check/legacy_accents) (com.google.fonts/check/legacy_accents)
⚠ WARN: Check for codepoints not covered by METADATA subsets. (com.google.fonts/check/metadata/unreachable_subsetting)
Use -F or --full-lists to disable shortening of long lists. Or you can add the above codepoints to one of the subsets supported by the font: ⚠ WARN: Glyphs are similiar to Google Fonts version? (com.google.fonts/check/production_glyphs_similarity)
Use -F or --full-lists to disable shortening of long lists. ⚠ WARN: Is there kerning info for non-ligated sequences? (com.google.fonts/check/kerning_for_non_ligated_sequences)
⚠ WARN: Ensure fonts have ScriptLangTags declared on the 'meta' table. (com.google.fonts/check/meta/script_lang_tags)
⚠ WARN: Glyph names are all valid? (com.google.fonts/check/valid_glyphnames)
⚠ WARN: Check font contains no unreachable glyphs (com.google.fonts/check/unreachable_glyphs)
⚠ WARN: Check math signs have the same width. (com.google.fonts/check/math_signs_width)
Width = 322: ⚠ WARN: Ensure soft_dotted characters lose their dot when combined with marks that replace the dot. (com.google.fonts/check/soft_dotted)
The dot of soft dotted characters should disappear in other cases, for example: į̆ į̇ į̈ į̊ į̋ į̒ į̦̀ į̦́ į̦̂ į̦̃ į̦̄ į̦̆ į̦̇ į̦̈ į̦̊ į̦̋ į̦̌ į̦̒ į̧̀ į̧́ Your font fully covers the following languages that require the soft-dotted feature: Dutch (Latn, 31,709,104 speakers), Lithuanian (Latn, 2,357,094 speakers). Your font does not cover the following languages that require the soft-dotted feature: Navajo (Latn, 166,319 speakers), Basaa (Latn, 332,940 speakers), Lugbara (Latn, 2,200,000 speakers), Ebira (Latn, 2,200,000 speakers), Avokaya (Latn, 100,000 speakers), Dan (Latn, 1,099,244 speakers), Belarusian (Cyrl, 10,064,517 speakers), Koonzime (Latn, 40,000 speakers), Ma’di (Latn, 584,000 speakers), Ejagham (Latn, 120,000 speakers), Aghem (Latn, 38,843 speakers), Igbo (Latn, 27,823,640 speakers), Ukrainian (Cyrl, 29,273,587 speakers), Kom (Latn, 360,685 speakers), Nateni (Latn, 100,000 speakers). [code: soft-dotted] ℹ INFO: Show hinting filesize impact. (com.google.fonts/check/hinting_impact)
ℹ INFO: Font has old ttfautohint applied? (com.google.fonts/check/old_ttfautohint)
ℹ INFO: EPAR table present in font? (com.google.fonts/check/epar)
ℹ INFO: Is the Grid-fitting and Scan-conversion Procedure ('gasp') table set to optimize rendering? (com.google.fonts/check/gasp)
PPM <= 65535: ℹ INFO: Check for font-v versioning. (com.google.fonts/check/fontv)
ℹ INFO: Font contains all required tables? (com.google.fonts/check/required_tables)
ℹ INFO: List all superfamily filepaths (com.google.fonts/check/superfamily/list)
Summary
Note: The following loglevels were omitted in this report:
|
@simoncozens There are two new FontBakery fails, should they be ignored or fixed? |
The experimental fail is just... wrong. The second one we have discussed at length and my position remains that if the designer knows what they are doing, using legacy accent glyphs as components of a precomposed glyph is fine. So yes, please ignore both. |
I reviewed this with Behdad's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
3920071: [gftools-packager] Noto Sans Arabic: Version 2.011 added