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

initSystemFonts: avoid the 'Material Icons *' fonts for monospace. #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ourairquality
Copy link
Contributor

The 'Material Icons *' have a limited character set and are not
generally a suitable choice to match a 'monospace' font familty.

Use of the font-family monospace was failing badly on Linux when the 'Material Icons *' were available as these happened to be a best match for monospace and could be chosen and yet they have a very limited character set. This is a quick hack to work around this. This does not prevent the 'Material Icons *' being used if named, just not a choice for the 'monospace' font family.

Perhaps this could be a place to discuss a more general solution.

Might the search for a matching font be best to match the language that the font declared?

Should the mapping from the CSS named font family to a system font depend on the language?

Might it consider the character set, but wouldn't that that depend on the language? Are there fonts in other languages that could possible map to the CSS named fonts and not support ASCII, could that be a minimum?

There appear to be two somewhat separate font selection paths in operation in CR. One path mapping the CSS font description to a font, and another path searching the fallback fonts. Currently there in only one primary font preference choice and fallback font preferences for that, but is this rather more complex, could there be a primary font preference for each of the CSS named fonts families, each with its own fallback list?

There is the function setAsPreferredFontWithBias() from koreader, and might it be more appropriate to use that? Would this bias against fonts by name, bias against the symbol and icon fonts? Would this bias in favor of the user preferred font and then the preferred fallback fonts so that a user preferred monospace font would get priority?

Anyway this hack keeps CR presenting something reasonable on Linux for monospace, rather than garbage output, and identifies the issue.

The 'Material Icons *' have a limited character set and are not
generally a suitable choice to match a 'monospace' font familty.
@poire-z
Copy link
Contributor

poire-z commented Jan 29, 2021

Perhaps this could be a place to discuss a more general solution.

(Too many questions that make my head hurts :) Anyway, the current behaviour feels good enough to me, no real need to add complexity - in the rendering and in the UI code to allow selecting all that - and you can't really trust fonts anyway to state what they are - may be when FontConfig is used, but otherwise not, we can't tell a serif from a sans-serif font.)

@virxkane
Copy link
Collaborator

LVFontManager::checkFontLangCompat() can be used to check if there is a set of glyphs for a specific language. So far I have not thought about how specifically it can be applied here, but it seems that it definitely refers to this problem.

@Frenzie
Copy link

Frenzie commented Jan 29, 2021

could there be a primary font preference for each of the CSS named fonts families, each with its own fallback list?

For reference, here's the font selection dialog in modern Firefox.

Screenshot_2021-01-29_10-28-12

Here it is in Otter.

Screenshot_2021-01-29_10-29-22

@virxkane
Copy link
Collaborator

virxkane commented Aug 23, 2021

@ourairquality

--- a/crengine/src/private/lvfreetypefontman.cpp
+++ b/crengine/src/private/lvfreetypefontman.cpp
@@ -401,7 +401,8 @@ bool LVFreeTypeFontManager::initSystemFonts() {
             css_font_family_t fontFamily = css_ff_sans_serif;
             lString32 face32((const char *)family);
             face32.lowercase();
-            if ( spacing==FC_MONO )
+            // Avoid using the 'Material Icons *' fonts for monospace.
+            if ( spacing==FC_MONO && face32.pos("icons") == 0 )
                 fontFamily = css_ff_monospace;
             else if (face32.pos("sans") >= 0)
                 fontFamily = css_ff_sans_serif;

Does it even work?
I found 5 fonts at https://github.com/google/material-design-icons. It's them?
So, face names in this font set is "Material Icons Sharp", "Material Icons", "Material Icons Two Tone", "Material Icons Outlined", "Material Icons Round". Thus face32.pos("icons") will never return 0 for this fonts.
May be you mean face32.pos("icons") == -1?
May be right condition must be face32.pos("material icons") == -1 or even face32.pos("material icons") != 0?
Also, now we already have conflict since #288.
I think we can accept this PR as a temporary solution with the above changes.

Perhaps this could be a place to discuss a more general solution.

Might the search for a matching font be best to match the language that the font declared?

Should the mapping from the CSS named font family to a system font depend on the language?

Might it consider the character set, but wouldn't that that depend on the language? Are there fonts in other languages that could possible map to the CSS named fonts and not support ASCII, could that be a minimum?

For more common solution you can see my experimental feature for Android frontend to filter available fonts: 60c6dbe from #307 that use #302.
Of course, this is not entirely consistent with the purpose of this PR, but perhaps it will help you implement your own solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants