Skip to content
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

[Android] Don't ship Noto #5310

Merged
merged 8 commits into from Sep 4, 2019

Conversation

@Frenzie
Copy link
Member

commented Sep 3, 2019

Follow-up to #5252. This greatly reduces the Android package size.

See discussion in #5264 (comment).

[Android] Don't ship Noto
Follow-up to <#5252>. This greatly reduces the Android package size.

See discussion in <#5264 (comment)>.

@Frenzie Frenzie added the Android label Sep 3, 2019

@Frenzie Frenzie added this to the 2019.09 milestone Sep 3, 2019

for _k, _v in ipairs(fonts) do
if _v:find(realname) then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

I was so confused why this wasn't working for a little while until I remembered - is a special character. One which of course doesn't occur in DroidSansMono, but does in DroidSans-Bold/DroidSans-Regular

Frenzie added 2 commits Sep 3, 2019

@Frenzie Frenzie requested a review from NiLuJe Sep 3, 2019

local ok, iter, dir_obj = pcall(lfs.dir, dir)
if not ok then return end
for f in iter, dir_obj do
if lfs.attributes(dir.."/"..f, "mode") == "directory" and f ~= "." and f ~= ".." then
_readList(target, dir.."/"..f)
else
elseif lfs.attributes(dir.."/"..f, "mode") ~= nil then

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 3, 2019

Member

-> == file? Or could this arguably hit a symlink one day?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

Possibly on regular desktop, but good point.

@@ -10,30 +12,30 @@ local logger = require("logger")
local Font = {
fontmap = {
-- default font for menu contents
cfont = "noto/NotoSans-Regular.ttf",
cfont = "NotoSans-Regular.ttf",

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 3, 2019

Member

Isn't losing the subdirectory an issue on !Android, where they do live in a noto subdirectory?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

The fonts are scanned in ./fonts/* first. :-)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

Oh drat, my reversal commit introduced an issue here sometimes skipping that scan. Meh.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

@NiLuJe The logic could be slightly different, i.e. try with full path and if fail use util.spiltFilePathName to search all folders. It'd be negligibly faster on platforms that ship all fonts; it's more the future readability of the logic that I'm trying to figure out.

@Frenzie Frenzie changed the title [Android] Don't ship Noto WIP [Android] Don't ship Noto Sep 3, 2019

@Frenzie Frenzie changed the title WIP [Android] Don't ship Noto [Android] Don't ship Noto Sep 3, 2019

local ok, iter, dir_obj = pcall(lfs.dir, dir)
if not ok then return end
for f in iter, dir_obj do
if lfs.attributes(dir.."/"..f, "mode") == "directory" and f ~= "." and f ~= ".." then
_readList(target, dir.."/"..f)
else
elseif lfs.attributes(dir.."/"..f, "mode") == "file" then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

@NiLuJe Actually on this note, on newer Android Droid is Roboto, so it's not just theory:

lrw-r--r-- root     root              2017-07-12 21:51 DroidSans-Bold.ttf -> Roboto-Bold.ttf
lrw-r--r-- root     root              2017-07-12 21:51 DroidSans.ttf -> Roboto-Regular.ttf

But I can change it to something like:

Suggested change
elseif lfs.attributes(dir.."/"..f, "mode") == "file" then
elseif lfs.attributes(dir.."/"..f, "mode") == "file" or lfs.attributes(dir.."/"..f, "mode") == "link" then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

Where by "newer" I presumably mean 4.0 and up. (I can only test 2.3 and 4.1 and up because the 4.0 emulator has always been borked.)

Edit: correction, I can't test a thing but it's working just enough to run adb shell. Which confirms it's already symlinked to Roboto there.

$ adb shell
# cd /system/fonts
# ls -l
-rw-r--r-- root     root         4824 2017-04-04 00:12 AndroidClock.ttf
-rw-r--r-- root     root         4824 2017-04-04 00:12 AndroidClock_Highlight.ttf
-rw-r--r-- root     root         4824 2017-04-04 00:12 AndroidClock_Solid.ttf
-rw-r--r-- root     root         6880 2017-04-04 00:12 Clockopia.ttf
-rw-r--r-- root     root       366760 2017-04-04 00:12 DroidNaskh-Regular.ttf
lrw-r--r-- root     root              2017-04-04 00:12 DroidSans-Bold.ttf -> Roboto-Bold.ttf
lrw-r--r-- root     root              2017-04-04 00:12 DroidSans.ttf -> Roboto-Regular.ttf
-rw-r--r-- root     root        13856 2017-04-04 00:12 DroidSansArmenian.ttf
-rw-r--r-- root     root       227928 2017-04-04 00:12 DroidSansEthiopic-Regular.ttf
-rw-r--r-- root     root      5300184 2017-04-04 00:13 DroidSansFallback.ttf
-rw-r--r-- root     root        21096 2017-04-04 00:12 DroidSansGeorgian.ttf
-rw-r--r-- root     root        30280 2017-04-04 00:12 DroidSansHebrew-Bold.ttf
-rw-r--r-- root     root        30024 2017-04-04 00:12 DroidSansHebrew-Regular.ttf
-rw-r--r-- root     root       119380 2017-04-04 00:12 DroidSansMono.ttf
-rw-r--r-- root     root        35584 2017-04-04 00:12 DroidSansThai.ttf
-rw-r--r-- root     root       185228 2017-04-04 00:12 DroidSerif-Bold.ttf
-rw-r--r-- root     root       190304 2017-04-04 00:12 DroidSerif-BoldItalic.ttf
-rw-r--r-- root     root       177560 2017-04-04 00:12 DroidSerif-Italic.ttf
-rw-r--r-- root     root       172916 2017-04-04 00:12 DroidSerif-Regular.ttf
-rw-r--r-- root     root        79620 2017-04-04 00:12 Roboto-Bold.ttf
-rw-r--r-- root     root        82880 2017-04-04 00:12 Roboto-BoldItalic.ttf
-rw-r--r-- root     root        82580 2017-04-04 00:12 Roboto-Italic.ttf
-rw-r--r-- root     root        79396 2017-04-04 00:12 Roboto-Regular.ttf

For comparison, here's 2.3 (not that it's relevant to our purposes):

$ adb shell
# cd /system/fonts
# ls -l
-rw-r--r-- root     root         6880 2017-01-18 00:31 Clockopia.ttf
-rw-r--r-- root     root       194488 2017-01-18 00:31 DroidSans-Bold.ttf
-rw-r--r-- root     root       190776 2017-01-18 00:31 DroidSans.ttf
-rw-r--r-- root     root        35880 2017-01-18 00:31 DroidSansArabic.ttf
-rw-r--r-- root     root      3725920 2017-01-18 00:31 DroidSansFallback.ttf
-rw-r--r-- root     root        23076 2017-01-18 00:31 DroidSansHebrew.ttf
-rw-r--r-- root     root       119380 2017-01-18 00:31 DroidSansMono.ttf
-rw-r--r-- root     root        36028 2017-01-18 00:31 DroidSansThai.ttf
-rw-r--r-- root     root       185228 2017-01-18 00:31 DroidSerif-Bold.ttf
-rw-r--r-- root     root       190304 2017-01-18 00:31 DroidSerif-BoldItalic.ttf
-rw-r--r-- root     root       177560 2017-01-18 00:31 DroidSerif-Italic.ttf
-rw-r--r-- root     root       172916 2017-01-18 00:31 DroidSerif-Regular.ttf

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 3, 2019

Member

Ah, nice find ;).

I'm wondering if there are any other possible values for attributes besides dir/file/link? (In which case the original test was perfectly fine ;p).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

https://keplerproject.github.io/luafilesystem/manual.html

string representing the associated protection mode (the values could be file, directory, link, socket, named pipe, char device, block device or other)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 3, 2019

Member

Ah, thanks, it always takes me forever to grok the lfs docs ;).

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 3, 2019

Author Member

So I don't think any of the others should be relevant?

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 3, 2019

Member

Yup! 👍

@pazos

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

With this PR merged and the new apk below 20MB I think it shouldn't hurt to migrate some stuff to Kotlin. It adds 800kb to the final APK and about 1-3% of extra build time but reduces boilerplate code, looks nice and has safer constructs. See an example: pazos/android-luajit-launcher@06658be

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

I have no real opinion. Kotlin is certainly less verbose just to get something done, which is nice of course.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Can't really be worse than plain java, so, yeah, +1 ;).

@pazos

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Can't really be worse than plain java, so, yeah, +1 ;).

A few killer features IMO:

compare how-to deal with NullPointerException mess

Java:

if (something != null) {
    something.run();
}

Kotlin:

something?.run()

In java you can do something.run() without checking if something is null. It will throw an amazing runtime error. In kotlin trying something.run() won't compile if something is nullable (and you can't assign a null value to non-nullable objects).

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

Their code samples are terrible though. I had to turn on their JS just to see them! (A proper JS syntax highlighting design just builds on top of <pre><code> so that it's always visible.)

@Frenzie Frenzie merged commit 2cd9b50 into koreader:master Sep 4, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:android-no-noto branch Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.