Allow overriding font-based normal line-height#12483
Conversation
|
I'm in the "patched font" camp, but this certainly looks like something nice to have, especially for like-minded OCD font nerds ;). |
I think they might be trying to say in a roundabout manner that most fonts do so. |
|
Is this going to be just an option, which is off by default? I spent too much time with Profiles adjusting each font with its line spacing to have exactly the same number of lines :D I don’t wanna do this again |
|
A few thoughts: I understand your aim, I guess we are all bothered by some fonts being nice, but having non-standard (non-what-we-are-used-to) line height. (I had to skip that upgrade to Bitter because of I understand there can be some maths to compute a thing that work. I'm just not sure: That it really works in all cases. I guess that for a same font size, you would then not use the font creator specified line height, but will use the user-set font size. I think that even at a same font size, a font "body" height (ie. the "x" height or an uppercase "B" height) could vary, so in effect impacting the visual vertical spacing between lines of text, so the perceived line height. Right ? But I get that at least, it will ensure that the nb of lines per page stays fixed (which is another kind of OCD, the one that affects @mergen3107 :)) Your formula is some competent people choice - but it's still just a taste of these people. It's nothing "per specs". It feels a bit odd to push that into crengine. You are pushing a 1.05em into crengine, but allow to set the "extra" ( Also, given that the user set font size is how he wants to read it, and that at most, it is scaled by a publisher by 80% to 120%, a user chosing 18 will get most text at a size around 18, and never 44 (except for the book title may be), and mostly may be 25 or 30 for chapter titles (which are mostly one line, the line height won't matter much). So, you could push a fixed preferred line height of 1.05*18 + your scaleBySize(3px) if you want - that you would just scale according to the Or you could autocompute (from frontend) the optimal (according to your formula) "Line spacing" depending on the user set font size, scaleBySize, the default font, and you would get 104% with one font, 97% with another. Dunno if that would work :) just throwing the idea. I'm also not fond of having this using a crengine prop, it means we'd need another toggle/number picker in frontend, dunno where we would put that. |
moben
force-pushed
the
normal-line-height
branch
2 times, most recently
from
cf85e23 to
fad468b
Compare
|
Thanks for the feedback everyone!
I don't really understand this point, maybe I'm missing context for when to use or not use props. It just seemed like the correct thing for such a global property. A private css property would also work but it's a bit weird if My main goals were:
I dropped the magic formula and constants, I agree it's hard to please everyone there. Instead I just exposed this as a setting:
Does this look better? Not sure if it should be per-document, but it would probably need to be integrated with profiles. |
Because you decided to make it a global property. I suggested other ideas just for the sake of discussion (which hasn't happened). Anyway, all in all, your current koreader/crengine#603 is probably the least crengine-intrusive way to implement this. int em = font->getSize();
if (gRenderNormalLineHeight > 0) {
int rem = enode->getDocument()->getDefaultFont()->getSize();
// Scale slightly to font size. Line height is partially a fixed offset and
// partially relative to font size, so scale half of the offset.
// Larger font sizes need less line height
line_h = em + (gRenderNormalLineHeight - rem) * (em + rem) / (2 * em);
}
else {
line_h = font->getHeight(); // line-height: normal
}I don't really get your Anyway, as said, I'm not strongly opposed to having that crengine PR, and even your frontend PR (even if making this menu 10 items).
You haven't answered that one. |
|
I need to compile by hand to test. I am not with my compiler machine yet, will try a little later. |
|
Sorry I haven't answered all your points in detail @poire-z, will try to do so below. That being said, I think you were right in your first comment that this was too opinionated, which is the initial feedback I was looking for.
What I mean is that semantically, this is a global property. It could of course be changed to be a local property, but it might get weird if it's adjusted only for some elements.
I'm also not sure, patched fonts or manually adjusting line heights in profiles might be too small a niche. But at least for me, the whole reason to do this is to make it accessible in a menu. If you don't know how to patch fonts, you currently need a style tweak that globally forces the line height. In my eyes, a simple tweak that sets
Making it a private css property, e.g.
Yes, that's my intention with this. I'll skip over the discussion of the originally proposed formula, I got rid of that and I think it's better this way. This also means there's no explicit relation to
Actually, What this is supposed to do is scale half of the whitespace by the local font size. So e.g. (assuming screen scaling 1:1):
Then:
The intention here is to scale a bit for the ±20% font size differences that you noticed but overall don't change too much from what the user set. While the fully magic formula that I originally proposed is a bit opinionated, there is truth in the idea that line-height can't scale linearly (inversely) with font-size. If there was a line in the same document at size 11, it should not have twice the line-height. On the other hand, this needs to ensure that even large font sizes (e.g. title page) don't get line-height 0. I hope I covered all points that were brought up so far. I too would be interested if this helps someone else's use case! For frontend, maybe I should investigate a way to allow style tweaks to have a parameter? Or maybe that already exists and I missed it? I would actually prefer if this could be a style tweak, it thematically fits better there and I'm not really married to the menu placement or prop. |
I'm not sure what you mean by that exactly, but of course you could easily set some text dynamically as part of the string. |
But there are no style tweaks that allow user-input, right? At least I couldn't find any. There's only on/off and e.g. "In-page footnote font size" just has a selection of percentages to choose from. |
|
Indeed, the general concept is that if you find the default ones insufficient you'll probably be more satisfied writing your own. |
Well, it's still you deciding it's semantically a global property. With a style tweak, one could target on purpose some specific element, or avoid targeting others.
Well, before you proposed that, nobody complained, they were all just fine using our
Thanks for the explanation and the examples. Yes, that's fine (I didn't really look at the frontend PR to see your were doing
Yes, that would be welcome, thanks. Been testing the crengine changes with just: --- a/frontend/document/credocument.lua
+++ b/frontend/document/credocument.lua
@@ -1221,6 +1221,7 @@ function CreDocument:setFontSize(new_font_size)
if new_font_size then
logger.dbg("CreDocument: set font size", new_font_size)
self._document:setFontSize(new_font_size)
+ self._document:setIntProperty("crengine.render.normal.line.height", 1.2*self:getFontSize())
end
end
Again, even if I've just shown above why a styletweak could be better, and the possible issue with math fonts, I still prefer the simplicity of the crengine PR as-is. I'd rather not review a bunch of CSS parsing additions for such a little odd feature. I've tested it on our "Generate font test document", and it is actually quite nice, getting rid of the variations and helping better comparing font (but not really :) we're no longer comparing their creator line heights :)) |
| checked_func = function() | ||
| return G_reader_settings:nilOrTrue("cre_size_based_normal_line_height") |
There was a problem hiding this comment.
We surely don't want this enabled by default and shuffling users' living room and habits.
| help_text = _([[ | ||
| Automatically set line height based on font size, instead of font metrics. | ||
| ]]), | ||
| separator = true, |
There was a problem hiding this comment.
The help_text will need a bit more explanation (and be different).
No separator=true needed between your 2 related menu items.
| value_min = 1, | ||
| value_max = 5, |
There was a problem hiding this comment.
1 was usable, so maybe allowing smaller values for people that like their life condensed could be nice.
| G_reader_settings:saveSetting("cre_normal_line_height", spin.value) | ||
| self.ui.document:setNormalLineHeight(G_reader_settings:nilOrTrue("cre_size_based_normal_line_height")) |
There was a problem hiding this comment.
I think these 2 related setting should have a same prefix, ie.:
cre_size_based_normal_line_height_enabled
cre_size_based_normal_line_height_value or _factor
| if new_font_size then | ||
| logger.dbg("CreDocument: set font size", new_font_size) | ||
| self._document:setFontSize(new_font_size) | ||
| self:setNormalLineHeight(G_reader_settings:nilOrTrue("cre_size_based_normal_line_height")) |
There was a problem hiding this comment.
Feels a little odd to pass one of the 2 settings as arg, and use the other one inside the called function.
|
Are the fonts used for menus going to be with minimal line heights? One screenshot above looks scary :D menu items are too condensed |
moben
force-pushed
the
normal-line-height
branch
from
fad468b to
d43e307
Compare
|
As was suggested multiple times, I just made this a style hint now, with a bunch of preset options to choose from, similar to the footnote font size style hints. I also got rid of the maths that were still there, in all my testing the difference this makes is so miniscule that it's not worth it in my opinion. If someone needs different "normal" line height for different elements (other than a constant factor), this can now be achieved via css.
no, that's just the emulator sometimes not scaling the menu correctly, I think it's related to running in a tiling window manager.
This version has slightly more changes in crengine, but it's all boilerplate to copy the css property around, have defaults and inherit it. The actual parsing is identical to I hope this is acceptable, you really convinced me now that a style tweak is better. I kept the old version for now for comparison, history can obviously be cleaned up. I'll away for two weeks and might not have time to check here frequently but will continue afterward if there are more suggestions. Still interested if this solves anyone else's problem as well. |
| }, | ||
| (function() | ||
| local sub_table = { | ||
| title = _("Override normal font line leight"), |
There was a problem hiding this comment.
Might need a better label, because if I hadn't read the PR, this would just look to me like it's simply enforcing the actual standard line-height CSS property ;).
That's fine, it lets us some time to make a stable release, and bring your new stuff only in followup nightlies.
Looks ok. I had in mind earlier when reviewing another thing that I sometimes wished we had: a way to target nodes according to their computed style (to fix/kill it), something like: *, autoBoxing {
-cr-hint: late;
-cr-only-if: (line-height: normal); /* or (line-height: >1.2) */
line-height: 1.2 !important;
}but that would need some heavy thinking about parsing & storing & checking the condition. For now, we have a bit of that with static targetting (that I added mostly for footnotes "fix-up"), and you could just have added another static string, and you could use: *, autoBoxing {
-cr-hint: late;
-cr-only-if: line-height-normal;
line-height: 1.2 !important;
}which would be handled like our May be this is still the way to go (new strings for the little needs we may have), and would avoid having to add new -cr- properties and to think about the combinations/inheritance between line-height and -cr-normal-line-height ? |
| title = T(_("Normal line height: %1"), rem), | ||
| css = T([[ | ||
| body { | ||
| -cr-normal-line-height: %1 !important; | ||
| } | ||
| ]], rem), |
There was a problem hiding this comment.
For small singles ones like this, you can have it on a single line, and no need for !important as it is a private properties no publisher will be using.
css = T([[body { -cr-normal-line-height: %1 }]], rem),
(also, rem feels like not the right variable name, num feels ok)
The crengine issues were fixed now I believe? But there's apparently still some weirdness with the terminal plugin on Android I (or someone) would have to look into. |
Yes, things should be stable on the crengine side. |
https://www.gutenberg.org/browse/languages/ru
Easy! Bottom menu,
I have a vague understanding that say Amazon is using this kind of methodology for their reading apps. Because in Kindle UI you only have three line spacing options for a very wide range of font sizes and none of them look terrible honestly :D |
|
I can try this, but my main gripe with fonts is actually not the line spacing, but the "height" of characters and how they define them. For example, compare Bookerly and Academy Old fonts. I have to increase Academy Old's actual font size to make "visible height" equivalent with that of Bookerly. Is it possible to calculate "average visible height" of characters (says, only a-z, or equivalent, or A-Z) and make crengine scale that instead? Then, with both these options under control ("average visible height", and line spacing from this PR) it will be much easier to achieve consistent results. |
This reverts commit 2af3e4bb8941bc12061bd9e2302114111eae1828.
moben
force-pushed
the
normal-line-height
branch
from
d43e307 to
884885a
Compare
I agree this is slightly at odds with the percentage-based line spacing menu. But when comparing to desktop browsers the default behavior is definitely correct and everyone convinced me that this fits in best as a style tweak. But having the line spacing setting just set the
I've never used a kindle or any reader other than koreader and plato really, but that's interesting to hear. Do their line spacing settings result in the same spacing for each font (and also custom ones)? And do they just override all line-spacing specified in css? Maybe there is more room for improvement. I tried looking through old issues but I don't really have the historical context for why line spacing is set up percentage based in koreader.
That might be possible, font metrics probably have Generally, the expectation is that most uppercase letters match the font size, but height of lower case letters varies more. As suggested, I moved the style tweak implementation here to a Settings now look like this:
|
I don't know for sure since java code is obfuscated, but it seems they 1) scale line spacing with font size; 2) also scale (or take into account) css-specified line-heights too. But one thing is certain, the same fonts I use on PC (MS Word) do not look the same in Kindle UI at all, they definitely do something |
|
If we're talking fonts shipped by Kindle a potential alternative is that they normalize them in the fonts themselves. |
moben
changed the title
You've got a typo in your "fix typo" commit. :P |
6 participants
Allow setting
normalline-height based on font sizeThis likely needs a bit more work to improve the implementation, hence the draft status (see todo below). Also depends on koreader/crengine#603 but I wanted to ask for some feedback if this is even worth pursuing / has a chance of being merged.
Motivation
Fonts have wildly varying
when
line-heightis set tonormalkoreader / crengine uses leading from the font metrics. As far as I can tell this is the correct thing to do and matches browser behavior. [1]However, fonts define wildly varying leading even at the same font size. If one wants smaller line spacing or consistent line spacing when switching fonts, there are a few options:
body { line-height: 1.2; }I'm currently using option 2 but wanted to explore if there's a way to get something like this built-in.
Proposal
As an opt-in tweak, allow specifying that
line-height: normalshould yield a consistent line height based on screen and font size.Common guidance and some experimentation suggest that good values depend on the font size, e.g.
line-height: 1.5might be good forfont-size: 12px, but forfont-size: 44px,line-height: 1.1can be enough. [2]A multiple of the font size plus a fixed offset gives line heights that match this guidance. e.g. this article suggests
2ex + 4px.Implementation
To the user this feels like a style tweak, but it can't really be one as far as I can tell because a reasonable line-height depends on the font size on that line. So even if style tweaks could be dynamic based on other settings such as font size, they could not take into account what font size is set for a specific line via css.
I originally tried to do this purely in crengine with just a toggle in frontend, but that doesn't work due to the screen-font-size scaling. crengine doesn't know that the
47pxfont size it sees is actually22pxscaled by screen size, so if it calculates4px + 2exthat yield a line height of1.09instead of1.18, which is too small. The4pxneeds to be scaled by screen size as well.The current approach presented here works well for me, but it feels like something cleaner should be possible. Maybe crengine should know the screen scaling factor (e.g. via prop) and apply it when setting the font size? Then the same scaling could be applied to this. I'm open to alternative suggestions.
Screenshots
Using Standrard Ebooks The Count of Monte Cristo and a few different fonts at size 22
Default
Size-based
line-height: normalTODO
-crcss properties that can be overridden using css tweaks for both fixed and scaling parts.1em == 2ex, which isn't true in desktop browsers[1] https://developer.mozilla.org/en-US/docs/Web/CSS/line-height says that desktop browsers default to a value of roughly
1.2, but this seems to be outdated. Both Firefox and Chrome use what the font defines. See also w3c/csswg-drafts#1254[2] https://fonts.google.com/knowledge/using_type/choosing_a_suitable_line_height#setting-line-height
This change is