search

koreader / koreader-fonts

Fonts for use in KOReader
6 stars 10 forks source link

Another round of font updates #15

Closed NiLuJe closed 3 years ago

NiLuJe commented 3 years ago

This change is Reviewable

NiLuJe commented 3 years ago

Ping @poire-z to double-check if the CJK fonts behave or if we need to stick to the old one ;).

poire-z commented 3 years ago

Mhhh, I don't really know/remember what I would have to check :/ The time when I caught something was rather by luck - you were updated the fonts while I was doing something in cre (don't remember what) that reveal the issues.

Thinking hard... remembering hard...

I remember something about a japanese middle dot. So, possibly the support for lang-tag/typography and switching between JA, and simplified/traditional chinese. Should also check that the symbols used with headings in Wikipedia EPUBs look the same (bigger that from other fonts, nicely vertically centered). There was another thing with diacritics badly aligned, possibly when decomposed into 2 unicodes, a font or hardbuzz picking a totally unrelated glyph - but no idea about on which of my test file this happened :/

NiLuJe commented 3 years ago

I definitely remember the middle dot thingy, yeah. There was a screenshot, and it probably was in the PR that bumped the font, if that can help you locate a test file?

poire-z commented 3 years ago

Pinging @WaseemAlkurdi @Zeyadas @Monirzadeh

Is the new rendering with the new font (on your right, sorry, should have put the new stuff on left for you RTL readers :) not worse that the old one ?

Old | New:

image

image

Some diacritics seem to be now higher than before - and on the last list, there's one word less (put onto next page), so some stuff may have become wider.

With a smaller font size: image

WaseemAlkurdi commented 3 years ago

@poire-z Looks all good to me! However, we should also test it on the actual KOReader UI, where there's much less vertical headroom - it might result in diacritics being shaved off (or might not - hoping for the best!)

NiLuJe commented 3 years ago

Except for Naskh, we're using the UI variant, they should already be doing their best at optimizing vertical headroom ;).

poire-z commented 3 years ago

@NiLuJe : I seem to have the same issue than at https://github.com/koreader/koreader-fonts/pull/6#issuecomment-619373571 when using Typography language> Japanese with the file below.

But it's a bit of a mess with fonts vs myfonts/ directory between appimage 2021.01 and my emulator - I think I cleaned that up and don't have the issue with 2021.01 but I have it with the fonts from this PR.

Here's my test file if you want to double check: test-urdu-nastaliq-but-actually-japanese.html.txt

And the one used for the arabic screenshots, but showing many other languages/scripts: test-scripts-r12a-phrases.html.txt

poire-z commented 3 years ago

we should also test it on the actual KOReader UI, where there's much less vertical headroom

Looks fine I guess ? Seems they didn't move the diacritics up with Noto Sans Arabic UI:

Before | After (if I didn't mess up): image image

WaseemAlkurdi commented 3 years ago

we should also test it on the actual KOReader UI, where there's much less vertical headroom

Looks fine I guess ? Seems they didn't move the diacritics up with Noto Sans Arabic UI:

Before | After (if I didn't mess up): image image

This screenshot doesn't have diacritics ... can you show me the "About KOReader" one (both menu item and dialog)?

poire-z commented 3 years ago

This screenshot doesn't have diacritics ...

Pardon my arabic ! :) Aren't all these little dots diacritics ?

can you show me the "About KOReader" one (both menu item and dialog)?

image

Couldn't get my emulator UI to be arabic, git pull l10n and it shows feb 16. I had to ln -s ar ar_AA to have it work. https://github.com/koreader/koreader-translations has ar/ But we have: https://github.com/koreader/koreader/blob/6132e8c904c9e019bfd08e6b534306a9decd416d/frontend/ui/language.lua#L140 @Frenzie : isn't that a problem?

NiLuJe commented 3 years ago

@poire-z: Yep, confirmed. So, unless we switch to the TTC, I guess updating Noto CJK is out ;).

Current: jp_cur

This PR: jp_new

On the Arabic side:

Current: ar_cur

This: ar_new

poire-z commented 3 years ago

We'll probably have to switch to the TTC at one point, as this takes care of the issue, at the cost of three new families ;).

And have to switch fonts (and add to, and re-order fallbacks ?) when switching typography language ?! And losing ability with typography language switch with lang= tags. And loosing trust in Harfbuzz to do the right thing :( Looks like a bug in the fonts to me, so, better to not use them until it's fixed. And CJK people don't complain about the older fonts :)

NiLuJe commented 3 years ago

Well, JP should switch to the CJK JP family, SC should switch to CJK SC family, etc. I suspect that's what Google expects frontend code to do with the TTC ;).

But, yeah, annoying in our case ;).

poire-z commented 3 years ago

Well, I expect Google expects this only from frontend code that doesn't support OpenType, and have to (or the user) switch fonts to get the glyphs for the expected language. I think we already discussed that, can't find better than https://github.com/koreader/koreader-fonts/pull/6#issuecomment-619405602.

NiLuJe commented 3 years ago

Well, ironically enough, while that's true, they also recommend the various compilation variants only to GSUB-aware renderers (e.g., in their flowchart that's Adobe/Apple).

But, yeah, the OTF should output the right variant when asked for, and that in a non-broken manner ^^.

What's strange is that, if the JP variant is broken via GSUB-from-the-SC-by-default, I'd have expected it to be broken, period. But apparently the JP-by-default variant behaves, which is... annoying.

poire-z commented 3 years ago

Unrelated but seeing in the About screenshot above:

image

https://github.com/koreader/koreader/blob/995668ac027f8737aba8be3b1a567b108e1a1534/frontend/ui/elements/common_info_menu_table.lua#L47-L50

Should we split this string so we BD.wrap/ltr() each part ? Or should this be left to the translator (to tweak/wrap the url, if he either want to change this URL if we don't hardcode it?

NiLuJe commented 3 years ago

@poire-z: Can we cheap out and kill the trailing slash? ^^.

Frenzie commented 3 years ago

+1, no need for a trailing slash

On Fri, Feb 19, 2021, 05:03 NiLuJe notifications@github.com wrote:

@poire-z https://github.com/poire-z: Can we cheap out and kill the trailing slash? ^^.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koreader/koreader-fonts/pull/15#issuecomment-781800597, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABRQBNTB4ENC2X33RZNXULS7XPKPANCNFSM4X2Z4KSA .