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

fontlist: Cache categorized font info (for mupdf) #6786

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

ezdiy
Copy link
Member

@ezdiy ezdiy commented Oct 12, 2020

Info about each face (l10n, name, family, style etc) is
now cached offline, so fonts can be queried ahead of time.

Fixes #6763
needs koreader/koreader-base#1215


This change is Reviewable

@ezdiy ezdiy requested review from poire-z and Frenzie October 12, 2020 13:47
@@ -66,7 +67,8 @@ function ReaderFont:init()
-- defaults are hardcoded in credocument.lua
local default_font = G_reader_settings:readSetting("cre_font") or self.ui.document.default_font
local fallback_font = G_reader_settings:readSetting("fallback_font") or self.ui.document.fallback_fonts[1]
local text = v
local text = FontList:getLocalizedFontName(cre.getFontFaceFilenameAndFaceIndex(v)) or v
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might do the cre.getFontFaceFilenameAndFaceIndex(v) at the start of the for loop, as it's now done for every font, and it might (the default) also be done in the font_func.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About FontList:getLocalizedFontName() - I wonder if we need an option to not get/show the localized one (in case it is crap and people were fine with the regular names). We might add that later if needed.

Copy link
Member Author

@ezdiy ezdiy Oct 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field is surprisingly accurate (compared to almost everything else being notorious garbage), if I'd wager for user requested option, it would be an option to display filenames - as those quite often don't match the "fancy names" of ours now, so its difficult to discern which font is what on storage.

["otf"] = true,
}

local function _readList(target, tinfo, mark, dir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might give a better name to this tinfo (which is fontinfo in the caller, so why not fontinfo here too ?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to move this into a local lambda. There are now 3 variables passed in the recursive call, all of which are fixed state. Would be much better off being captured by the closure at this point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(split into util.findFiles in 670473d, as this was badly nested already)

@@ -119,13 +170,64 @@ end

function FontList:getFontList()
if #self.fontlist > 0 then return self.fontlist end
_readList(self.fontlist, self.fontdir)

local cache_file = DataStorage:getDataDir() .. "/cache/fontinfo.lua"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have said it's sad (although needed) to need to have and maintain a cache for that - because it's painful to code - but you seem to have done that really well, so ok :)

@Frenzie Frenzie added this to the 2020.11 milestone Oct 12, 2020
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine at a glance, but I won't have time to really read or test for the next two weeks. (Probably more like three, but in any case I shouldn't have to worry about moving then, only about unpacking. xP

@ezdiy
Copy link
Member Author

ezdiy commented Oct 12, 2020

@Frenzie It's not nearly as invasive as it seems, @poire-z and @NiLuJe having to babysit me will do fine :)

I'll wait with bulk of mupdf until then tho, as that one will need all the eyes due to the horrors around 3rd party building stuff/integration coordination of 4 separate repos etc.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 12, 2020

Yeah, I won't pretend that I grok everything involved in here, but it looks neat and orderly ;).

Info about each face (l10n, name, family, style etc) is
now cached offline, so fonts can be queried ahead of time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: [show CJK font full name instead font family name when preview fonts]
4 participants