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

bump crengine: more granular font weights #7616

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Apr 28, 2021

Includes:

I decided to not care about the migration from the old boolean font_embolden to the replacement number font_base_weight - this will let people who had bold know that there is this new setting with more granular weights, as they'll probably enjoy 1.5 more than the old "bold"=3.

image
(2 in the screenshots has been corrected to +2, too lazy to remake the screenshot)

I got rid of +3, I wanted to get rid of +2 but the toggle felt a bit naked then :)

Rewording suggestions welcome.

@yparitcher : any idea if we renaming/removing a dispatcher setting/action (font_weight) can cause issues if a user has been using it?


This change is Reviewable

@NiLuJe
Copy link
Member

NiLuJe commented Apr 28, 2021

Oh, I quite like what you settled on to show what the current font supports ;).

precision = "%+.2f",
value_hold_step = 1,
},
help_text = _([[Set the font weight delta from "regular" to apply to all fonts.
Copy link
Member

@NiLuJe NiLuJe Apr 28, 2021

Choose a reason for hiding this comment

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

That's technically a weight class, but since we're not actually displaying that to the user outside of the help/status, I'm okay with the simplification ;).

- 0 will use the "regular (400)" variation of a font.
- 1 will use the "medium (500)" variation of a font if available
- 3 will use the "bold (700)" variation of a font if available
If a font variation is not available, and for fractional deltas, a font will be synthesized from the nearest available weight of the font.]]),
Copy link
Member

Choose a reason for hiding this comment

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

as well as for fractional deltas

Copy link
Member

@NiLuJe NiLuJe Apr 28, 2021

Choose a reason for hiding this comment

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

Could also possibly keep the variation thing going, since that's going to be a term more and more commonly used with the advent of variable fonts, so, a font variation will be synthesized.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just a fractional adjustment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as well as for fractional adjustments (plural), right ?

help_text_func = function(configurable, document)
local font_face = document:getFontFace()
local available_weights = cre.getFontFaceAvailableWeights(font_face)
return T(_("The default font '%1' is available in these real weights: %2"), font_face, table.concat(available_weights, ", "))
Copy link
Member

@NiLuJe NiLuJe Apr 28, 2021

Choose a reason for hiding this comment

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

Oh, there I'd possibly use weight classes, though ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I keep the "real" in there ? in these real weight classes ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it feels slightly clunky, but I couldn't really think of anything better. Perhaps getting rid of it entirely does the trick, though, the context should make it clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"The default font 'Noto Sans' is available in 400, 700." ? Feels a bit short without "real", one could think +1 and +2 won't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But might be OK, above we state "If a font variation is not available, a font variation will be synthesized...".

Copy link
Member

@NiLuJe NiLuJe Apr 29, 2021

Choose a reason for hiding this comment

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

Oh, no, sorry, I meant just omitting "real" ;).

i.e., available in weight classes x, y, z

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh :/
Anybody feel free to correct this in his next PR if "available in 400, 700." feels like broken english :)

Copy link
Member

Choose a reason for hiding this comment

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

available in: x, y?

@poire-z
Copy link
Contributor Author

poire-z commented Apr 28, 2021

Updated help text with suggestions:

image

@poire-z
Copy link
Contributor Author

poire-z commented Apr 28, 2021

Does this +3 look odd, in the serie of numbers ?

image

+2, synthesized from the bold, is often thiner (so, surprising and soon forgotten) than the +1 1/2 synthesized from the regular - and we mention 3 in the help_text.
(Looks like I should add some + in the help text before the numbers.)

@Frenzie
Copy link
Member

Frenzie commented Apr 28, 2021

It does seem odd, but I'm not sure if there's an alternative.

@NiLuJe
Copy link
Member

NiLuJe commented Apr 28, 2021

Even if only just because the old setting exposed Bold, and not SemiBold, yeah, I'd keep 3 instead of 2 ;).

@poire-z
Copy link
Contributor Author

poire-z commented Apr 28, 2021

image

@poire-z
Copy link
Contributor Author

poire-z commented Apr 28, 2021

Or just "in 400 700" to avoid having to handle singular or plural "in weights 400." :/
image

Includes:
- MathML: a few minor fixes
- (Upstream) lvtext: fix possible index out of range
- Fonts: RegisterExternalFont() should take a documentId
- Fonts: fix: letter-spacing should not be applied on diacritic
- (Upstream) Fonts: more granular synthetic weights
- Fonts: synthesized weights: tweak some comments
- Fonts: keep hinting with synthetic weight
- Fonts: fix synthesized weight inconsitencies
- Fonts: fix getFontFileNameAndFaceIndex()
- Fonts: adds LVFontMan::RegularizeRegisteredFontsWeights()
- Fonts: handle synth_weight tweaks in glyph/glyphinfo slots
- (Upstream) Fonts: fix some compiler warnings
- Fix hyphenation on Armenian and Georgian text

Update the bottom menu widget "Font Weight" to allow more
granular weights than the previous "regular | bold".

Also bump thirdparty/luasec to v1.0.1.
@poire-z poire-z merged commit 9ef435c into koreader:master Apr 28, 2021
@poire-z poire-z deleted the bump_crengine branch April 28, 2021 23:37
@yparitcher
Copy link
Member

@yparitcher : any idea if we renaming/removing a dispatcher setting/action (font_weight) can cause issues if a user has been using it?

should be fine, #6785 should have fixed any issues.

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Apr 29, 2021
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Apr 29, 2021
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Apr 29, 2021
NiLuJe added a commit that referenced this pull request Apr 30, 2021
@poire-z poire-z mentioned this pull request May 8, 2021
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.

Hyphenation not working in Georgian text (Android)
4 participants