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

Update Google Fonts #3942

Merged
merged 3 commits into from May 16, 2018

Conversation

Projects
None yet
5 participants
@NiLuJe
Member

NiLuJe commented May 10, 2018

Bump to latest releases

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 10, 2018

Linked w/ BASE#672 to update our MuPDF patch to the new filename of NotoSansCJK-SC.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

Pinging... @frankyifei I believe. There's something about the CJK fonts but I forget what exactly.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

Oh, I was probably confused with #3564.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 10, 2018

I just had a quick check (to see if there were more usable unicode symbols than in previous version), and our pretty little checkboxes have grown ugly: edit: all is fine
image

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 10, 2018

Mhhh, may be using patch -p1 < 3942.patch is not a good idea when it contains "GIT binary patch" :)
OK, git apply 3942.patch is the right way to do that.
And symbols look OK again :) sorry for the noise.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 10, 2018

@poire-z : just to be clear, that's a local patch you apply on your own copy, not something I missed somewhere?

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 10, 2018

@NiLuJe : yes, I messed up applying your PR patch (fetched with wget by appending .patch to your PR url). Your work in this PR as-is seems fine :)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 10, 2018

Oh, right, 3942 is this PR's number. Duh! :).

@KenMaltby

This comment has been minimized.

KenMaltby commented May 10, 2018

That menu is a little confusing, shouldn't the "Swipe" options be radio buttons? Why the separation line before the last one? What dose the "open book" box signify?

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 10, 2018

@KenMaltby : that was not a change proposal :)
It was just me that had fonts corrupted, so our beloved checkboxes were corrupted and appeared as "open book" box as you put it (I wouldn't know how to make such an "open book" box symbol now that my fonts are fixed.
(The separation line has been there for a few months, I guess it's because even if it is swipe, is was no more about links, but about latest bookmark.)

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 10, 2018

@poire-z Well, there is this 🕮 UTF-8 character… :-P

@Frenzie

Probably best to wait for @frankyifei to respond with regard to CJK but naturally fine in principle.

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 11, 2018

I will update the font file and upload here. It seems 🕮 is not supported on my Android phone.

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 11, 2018

it's not CJK but Emoji, I added noto emojis in the font file. I did not test it but it should work.
NotoSansCJK-Regular-withEmoji.zip

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

@frankyifei: Aren't the various NotoSansCJK variants already hitting the 65535 glyph limit?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

More to the point, did we ever modify NotoSansCJK, and if yes, why?

I can't find any of the NotoEmoji glyphs in the previous version we shipped, for instance.

And the one you linked is unhinted, which is arguably not great, especially for CJK?

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 11, 2018

I deleted some of the not commonly used chinese characters. So there are less than 50,000 in this file.

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 11, 2018

It is modified several times to add support for other languages. Some one is asking for emoji so I just added emoji.
I dont know much about hinted fonts. This font was originally downloaded from google and combined with several language specific noto sans. I thought hinted fonts are optimized for computer screens, not for high ppi devices like kindle.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

Besides the hinting issues:

screenshot_20180511_174515

There's also a scale issue when merging NotoEmoji:

screenshot_20180511_174756

TL;DR: I'm at a loss at why we'd even want to modify NotoSansCJK SC in the first place?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

I'm a big fan of hinting, even on high-dpi screens ;).

Now, if the issue is actually missing script/language support, then that potentially qualifies as a valid reason to do weird stuff ;). (Because shipping the full set of CJK variants would take ~100MB).

So, to recap: do we need to tweak the latest CJK SC version, and if so, why?

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 11, 2018

At the begging someone is asking for support of Arabic or Hindu support and they want the fallback font contains these characters. Because we dont have font config support so the fallback font has to be one file that contains as many languages as possible

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

Ok, that's clearer, thanks ;).

Wouldn't shipping Noto Sans Devanagari (for Hindi) & Noto Sans Arabic seem like a better/simpler solution? (That's a few MB, at most).

Is the case where text is mainly in Latin script, with small portions/quotations in Hindi/Arabic without a specific font requested/embedded that common?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

I definitely get the 'most language possible in the fallback' argument, but in that case, we have a problem: the CJK font is the one supporting the least amount of languages ;).

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

And going back to DroidSansFallback doesn't really help, it's mostly CJK there, too.

The font with the most coverage we ship right now is Noto Sans. That still doesn't include Arabic or Hindi, though.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

And, understandably, as both those scripts are barely usable without a layout engine anyway, both Noto Sans Devanagari & Noto Sans Arabic do NOT cover Latin characters at all.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 11, 2018

@NiLuJe

I definitely get the 'most language possible in the fallback' argument, but in that case, we have a problem: the CJK font is the one supporting the least amount of languages ;).

Big sense of déjà vu incoming. I said something extremely similar last year or so. ;-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 11, 2018

The thing about this 🕮 UTF-8 character was a joke by Frenzie, not really a request to have it in our fonts.
But if we wanted emoji for koreader menus (which I don't think we need), we could add another fallback font in:

fallbacks = {
[1] = "noto/NotoSansCJK-Regular.ttf",
[2] = "noto/NotoSans-Regular.ttf",
[3] = "freefont/FreeSans.ttf",
},

But if emojis were beginning to appear in EPUBs :(, we may need them to be available in some font.
The thing is that there is only support for one fallback font in crengine. So, with the default font and the fallback font, there would be support for 2 alphabets if we don't extend them (do we need more? - and yes, there is the argument about less common languages would probably have embedded fonts in the EPUB).
I think FreeSerif is quite good with other languages (I used it as my fallback font, and arabic, hindi... display fine in most wikipedia EPUBs), and it was better than all the Noto (before this update).
(I actually merged NotoSansCJK into FreeSerif to have more coverage - not that I read any of these, but it's nicer in wikipedia EPUBs that don't include any embedded font).

But it feels a bit strange to have that work done manually by @frankyifei (how would we update if you were to disappear :| :|). Can't that be automated or documented? Or isn't there a smaller suit-all-languages font even if of less quality than Noto, that people could use as a fallback if they really needed that much coverage?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 11, 2018

IIRC, FontForge can be scripted, but I'm not familiar with how that actually works in practice... (Hell, even with the GUI, I'm mostly clicking on shiny-looking stuff more or less at random :D).

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 12, 2018

Okay! To recap: there's no magic bullet, so we have to make compromises somewhere.

I kind of like where this PR stands right now, which is basically:

  • No new fonts, minimal extra storage space needed
  • Vanilla fonts, so, easy to maintain.
  • Good coverage in the UI
  • Perfect LGC coverage w/ Noto Sans
  • Leaves to the user the responsibility of setting up the right fallback font for his reading in CRe, with a default that is only highly optimized for CJK readers, especially C. Which, granted, is probably a not-so-stupid decision, since that's probably our largest non-LGC user-base.
    • If need be, we can document FF merging somewhere, or point to Code2000/Unifont (without shipping them directly).
@poire-z

This comment has been minimized.

Contributor

poire-z commented May 12, 2018

With the fallback tweaks, we know get the smaller/thinner/misaligned heading symbols (from FreeSans or FreeSerif) in Wikipedia full pages (they are better in NotoSansCJK), but more correctly looking arabic (even if probably wrong as these should be RTL). I dislike compromises, but ok :)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 12, 2018

@poire-z : Which bits of the UI would that be, exactly?

Since Noto CJK's coverage is so specific, that might be easy enough to workaround: Move Noto CJK behind Noto Sans (i.e., Noto Sans -> Noto CJK -> Free Sans -> Free Serif).

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 12, 2018

The Wikipedia full page, which looks currently like that:
image
With Free* before NotoSansCJK:
image

Since Noto CJK's coverage is so specific, that might be easy enough to workaround: Move Noto CJK behind Noto Sans (i.e., Noto Sans -> Noto CJK -> Free Sans -> Free Serif).

Yes, that could work, now that we keep the vanilla Noto CJK without the added wrong arabic. Will test.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 12, 2018

Looks (as obviously, I can't read any) perfect, with China EN, where there is some chinese, persian, sanskrit.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 12, 2018

@frankyifei : anything you think we'd miss from the glyphs you added manually to the vanilla NotoSans CJK, if we keep the vanilla NotoSans CJK ?

@frankyifei

This comment has been minimized.

Contributor

frankyifei commented May 12, 2018

I added some emoji yesterday. Arabic can be handled in other fonts. I dont think there is something we need to worry about vanilla noto CJK.

@poire-z

This comment has been minimized.

Contributor

poire-z commented May 13, 2018

Shall we bump base for koreader/koreader-base#672 and merge this (haven't tested the mupdf patch, but this frontend change is fine with me).

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 13, 2018

The actual changes are great, but I'm not a big fan of adding the 14+ MB binary files to the main repo like I said. :-/

I like having the whole repo available rather than being forced to shallow clones by too many binary files in there, you see. :-)

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 13, 2018

@Frenzie : I genuinely don't understand the issue, but if you want to restructure the layout of the repo, go ahead and I'll rebase once that's done.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 13, 2018

@NiLuJe Try doing a full clone of this repo ;-) https://github.com/blchinezu/pocketbook-coolreader

The structure as such wouldn't change. It'd just be the fonts dir as a submodule (like base, test, and android).

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 13, 2018

We and the CI would still need to pull the submodule at one point, so I really, really don't see the difference :?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 14, 2018

I'd understand the interest if git submodules were shallow by default, but that's not the case.

And even then, it's a bit late, and switching now would probably do more harm than good, because we'd still carry the history in the main repo, but have no delta to rebuild the same data in the submodule.

Unless I'm missing something, which is more than probable, because git. :D.

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 14, 2018

We and the CI would still need to pull the submodule at one point, so I really, really don't see the difference :?

The GH online search interface is terrible. There's great value to me in quickly being able to pull in a relatively small repo completely (as opposed to only, say, 50-100 commits). You don't have to init submodules or anything to do that.

I'd understand the interest if git submodules were shallow by default, but that's not the case.

We can make it so for binary submodules like fonts and test. In fact that's exactly what I was thinking. :-)

And even then, it's a bit late, and switching now would probably do more harm than good, because we'd still carry the history in the main repo, but have no delta to rebuild the same data in the submodule.

What delta? Do these binary files have an actual diff overlap? ;-) The patch file is 35 MB.

Unless I'm missing something, which is more than probable, because git. :D.

The harm is adding another couple dozen MB to the repo on every font update. It would already make the repo more annoying to work with because it'd be larger, but past a certain point Git will start to slow down and take longer to do all of its lovely Git stuff if you bloat it too much. Luckily, for us that'd take a great many years but I still think it's something to beware of. Just because the repo I linked to may or may not be big enough to slow Git down yet doesn't mean it isn't incredibly annoying to work with.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented May 14, 2018

@Frenzie : Guess I was hoping the deltas were slightly less dramatic than they actually are (f.g., the small changes in Droid Sans Mono are dealt with in a fairly okay'ish manner ;)).

Feel free to split things off as you see fit, and I'll re-implement that in the right place once that's done ;).

@Frenzie

This comment has been minimized.

Member

Frenzie commented May 14, 2018

Cool! I'll create new repo called koreader-fonts and then I'll init that as a --depth=1 submodule in place of the current fonts/ folder. But I'll do that tomorrow; I don't have too much time atm (and none tonight). :-)

@Frenzie Frenzie referenced this pull request May 15, 2018

Merged

Add all fonts #1

Frenzie added a commit to koreader/koreader-fonts that referenced this pull request May 15, 2018

NiLuJe added some commits May 16, 2018

Minor cleanup
We don't ship Droid Sans Fallback anymore

@NiLuJe NiLuJe force-pushed the NiLuJe:master branch from 61a7119 to 54c43cd May 16, 2018

@NiLuJe NiLuJe referenced this pull request May 16, 2018

Merged

Update Google fonts #2

Bump base & fonts
To pickup related changes

@NiLuJe NiLuJe merged commit ebf4777 into koreader:master May 16, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment