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

Line Heights problem #4876

Closed
noembryo opened this issue Apr 6, 2019 · 37 comments

Comments

Projects
None yet
4 participants
@noembryo
Copy link

commented Apr 6, 2019

With v2019.04 there is a change in the way the text is presented with #4785.
I can see that a lot of work went on this change and to make it the right way, but it changed the way the books are displayed.

Up until the v2018.11.1 the display was this:
Reader_2019-Apr-06_144431
I was using DCREREADER_CONFIG_LINE_SPACE_PERCENT_SMALL = 90 and DCREREADER_CONFIG_DEFAULT_FONT_SIZE = 32 and the lines filled up all the screen.

With the v2019.04 the display is this:
Reader_2019-Apr-06_144317
There is a line missing at the bottom.

I tried different settings (changing line heights (80%-100% and/or font sizes (29-33) but always the bottom gap is big.

This is with a bigger line height:
Reader_2019-Apr-06_150018

I thought that there is a margin problem there, but I found that when there are different tags and not just <p> in the page I could almost fill all of it like this:
Reader_2019-Apr-06_152314

I wonder, is there a way I could recreate the page the way it was before?
A setting to use the old numbers? (I know that its nit-picking, but as you can see from the first screenshot, I don't like big margins.. ;o)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

You're saying that, e.g., 85 to reduce the line-height to the previous value still leaves a gap at the bottom?

Also note the widows and orphans, btw.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Using the same values as before does not work as expected.
Using smaller line height brings more lines to the page but the gap is still there..

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Aaa, and there are no widows and orphans..

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Using 85 height (the same lines as the first screenshot but with a gap):
Reader_2019-Apr-06_185652

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

The previous values were broken. You might need to use a smaller value to achieve the same result.

Using 85 height (the same lines as the first screenshot but with a gap):

The line-height there is smaller than in the first picture, hence the gap. :-)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

There is a line missing at the bottom.

This has always been a question of luck :) You've just been using some combination of margin and font size that happened to just fill the page height (like me: Bitter 19). Even before, if you just increased the font size, or change the margins, there was a good amount of chance that you get this empty space at the bottom where we can't fit a line.

Up until the v2018.11.1 the display was this: => 25 lines of text
With the v2019.04 the display is this: => 23 lines of text, just missing a few pixels for a 24th

Usually, this setting should make line-heights most often smaller, so, more lines :(
So, it may depends on the CSS styles you have in that book.

A setting to use the old numbers?

You can try Zoom/DPI: off, where I had it do (with some limits) the former (wrong) computation of line-height.

But I guess you'll have more chances to get what you want by tweaking the margins. Decrease the value for the bottom margin till you find what works with your favorite combination of font size & line height.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

The line-height there is smaller than in the first picture, hence the gap. :-)

Yes, I know, but I can't use 89 or 88 to fine tune it.. :o(

But I guess you'll have more chances to get what you want by tweaking the margins.

This has the same problem (lack of fine tuning values)

It would be great if I could add an Offset value in pixels for the line height or margin.
Also, the margin settings are for all sides, and this is not so flexible...

You can try Zoom/DPI: off, where I had it do (with some limits) the former (wrong) computation of line-height.

This also didn't change much :o(

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Copy that in a default.persistent.lua file at the root of koreader directory:

koreader/defaults.lua

Lines 112 to 121 in 22db71c

-- crereader margin sizes
-- margin {left, top, right, bottom} in pixels
DCREREADER_CONFIG_MARGIN_SIZES_SMALL = {5, 10, 5, 10}
DCREREADER_CONFIG_MARGIN_SIZES_MEDIUM = {10, 15, 10, 15}
DCREREADER_CONFIG_MARGIN_SIZES_LARGE = {20, 20, 20, 20}
DCREREADER_CONFIG_MARGIN_SIZES_X_LARGE = {30, 30, 30, 30}
DCREREADER_CONFIG_MARGIN_SIZES_XX_LARGE = {50, 50, 50, 50}
DCREREADER_CONFIG_MARGIN_SIZES_XXX_LARGE = {70, 70, 70, 70}
DCREREADER_CONFIG_MARGIN_SIZES_HUGE = {100, 100, 100, 100}
DCREREADER_CONFIG_MARGIN_SIZES_X_HUGE = {140, 140, 140, 140}

Decrease the last number, try even setting it to 0.
(We furthermore add DMINIBAR_HEIGHT to the bottom margin, so it should not overlap - although I just saw it overlapping while testing...)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Yes, I know, but I can't use 89 or 88 to fine tune it.. :o(

If so, I'd call that a bug. :-)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Finely tunable the same manual way:

koreader/defaults.lua

Lines 128 to 141 in 22db71c

-- crereader line space percentage
DCREREADER_CONFIG_LINE_SPACE_PERCENT_X_TINY = 70
DCREREADER_CONFIG_LINE_SPACE_PERCENT_TINY = 75
DCREREADER_CONFIG_LINE_SPACE_PERCENT_XX_SMALL = 80
DCREREADER_CONFIG_LINE_SPACE_PERCENT_X_SMALL = 85
DCREREADER_CONFIG_LINE_SPACE_PERCENT_SMALL = 90
DCREREADER_CONFIG_LINE_SPACE_PERCENT_L_SMALL = 95
DCREREADER_CONFIG_LINE_SPACE_PERCENT_MEDIUM = 100
DCREREADER_CONFIG_LINE_SPACE_PERCENT_L_MEDIUM = 105
DCREREADER_CONFIG_LINE_SPACE_PERCENT_XL_MEDIUM = 110
DCREREADER_CONFIG_LINE_SPACE_PERCENT_XXL_MEDIUM = 115
DCREREADER_CONFIG_LINE_SPACE_PERCENT_LARGE = 120
DCREREADER_CONFIG_LINE_SPACE_PERCENT_X_LARGE = 125
DCREREADER_CONFIG_LINE_SPACE_PERCENT_XX_LARGE = 130

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

(We furthermore add DMINIBAR_HEIGHT to the bottom margin, so it should not overlap - although I just saw it overlapping while testing...)

Somehow explainable, and compensated but the bottom margin that's added from the chosen margin, and this somehow ensures we don't see to much of this issue:

koreader/defaults.lua

Lines 155 to 156 in 22db71c

DMINIBAR_HEIGHT = 7 -- Should be smaller than DMINIBAR_CONTAINER_HEIGHT
DMINIBAR_CONTAINER_HEIGHT = 14 -- Larger means more padding at the bottom, at the risk of eating into the last line

I don't see any way to avoid these blank empty nearly line height at bottom. It all depend on what's on the page.
Use a small bottom margin and if you have 1 or 2 titles with a bigger font size, on this page, it will happen again.
We know the page height, but centering it would looks ugly when turning pages, and ghosting would be more noticable when lines aren't drawn over previous lines like it mostly happens currently. Not talking about pages which have a small height when they are the last in a chapter...

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

Now I can fix it (one way or another ;o)

Tell us how you'll have it fixed to your liking, just curious :)

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

You guys are great!

And it was my mistake in a way.
I was using an older defaults.persistent.lua and didn't check for changes from the original defaults.lua.
I missed these items.

Now I can fix it (one way or another ;o)

(Re-posting this with the right username...)

Tell us how you'll have it fixed to your liking, just curious :)

Probably by assigning different default values and scaling the others to make it consistent .. :o)

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Tell us how you'll have it fixed to your liking, just curious :)

Setting 89 (instead of 90) did it!
Thank you all :o)

@noembryo noembryo closed this Apr 6, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Another crappy workaround to bypass the can't-fit-the-last-line issue is to switch to scroll mode.

I don't like it, fine-tuning your settings so it works in page mode feels better, which is why we worked on making both line-heights and font-sizes more fine-grained, but, if you're in a hurry, that works, too ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Scroll mode works fantastic (and it should be called something like continuous instead to quit misleading new or possibly even experienced users) but it seems fairly pointless in EPUB.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 6, 2019

Yes, continuous is more accurate.

The problem with Scroll mode is that something is always cut.
Some letter tops, that look like dots at the end of the page and the rest of the letters in the next page.
You have to adjust the line height/font size, so that the cut happens between lines (which is equally difficult ;o)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

May be we could find some kind of rule for still letting a line on this page even if it exceeds a bit the page height, under some conditions.
https://github.com/koreader/crengine/blob/9a67796156b4136281fa7c6c7cfd2a862bc07686/crengine/src/lvpagesplitter.cpp#L370
There, we know the page height, we know the height of the line we want to add. A line in this context is any slice of the document height: some regular text, some larger text for a title, some margin, some padding (which includes the border) - but we don't know what it is at this point.

Like, if 90% of the line does fit, and the overflowed height is under 0.3rem (random numbers here, just for thoughts).
Some computer formulation of when we get the feeling that There is a line missing at the bottom :)

Or some tweaking of the influence of DMINIBAR_HEIGHT there (like don't include it when it's the bottom margin is already bit enough and contains it with plenty margin left):

if self.view.footer.has_no_mode then
bottom = Screen:scaleBySize(margins[4])
else
bottom = Screen:scaleBySize(margins[4] + DMINIBAR_HEIGHT)
end

(But nothing obvious or 100% succesfull while playing with hardcoding values here and there...)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Yeah, I'm on the team that'd prefer moderate clipping to blank spaces, so anything to play with that would be neat (knowing full well that there won't be a magic bullet answer that works every time, and that a 'no clipping allowed' behavior does seem the sanest default).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

@NiLuJe : what's your opinion about:

We know the page height, but centering it would looks ugly when turning pages, and ghosting would be more noticable when lines aren't drawn over previous lines like it mostly happens currently

in case we would allow overflowing a % of top + bottom margins, and eventually shifting the page up to balance the overflow in the top margin and avoid clipping at the bottom?
(I guess it happens often in Wikipedia EPUB, with there titles that hardly make ever lines laid out exactly over previous page's lines.)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@poire-z : I know it wouldn't bother me, and I'd personally probably quite like it ;).

Your concerns are valid, though (at least on !reagl devices), and I do wonder how noticeable that would be in practice...

Random related thought: do we currently have a way to ignore the minibar height entirely? I imagine I'm not the only one who usually reads with it disabled? (And the PDF reader is already dealing with it in a dubious to non-existent manner :D).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

I usually keep it on, but it tends to be mostly or entirely hidden from view by the bezel.

There's some configuration, but it'd probably negatively affect minibar display?

koreader/defaults.lua

Lines 155 to 156 in 22db71c

DMINIBAR_HEIGHT = 7 -- Should be smaller than DMINIBAR_CONTAINER_HEIGHT
DMINIBAR_CONTAINER_HEIGHT = 14 -- Larger means more padding at the bottom, at the risk of eating into the last line

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

do we currently have a way to ignore the minibar height entirely

It seems that is done when self.view.footer.has_no_mode, so when you uncheck everything in the Status bar submenu.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

@poire-z : Which is not the same thing as just hiding the status bar, IIRC.

Might be nice to have a way to enforce this behavior even with the status bar potentially able to be toggled on via taps ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

The issue then is that you would change the cre view WxH, and that should trigger a full book rerendering (just like when you change font size or line height) because lines could then be splitted differently among pages, meaning a possible >1mn wait on big books, just because you taped by error on the bottom area :|

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Then, just have another setting or DMINIBAR_SOMETHING so the first if is taken in all cases in:

if self.view.footer.has_no_mode then
bottom = Screen:scaleBySize(margins[4])
else
bottom = Screen:scaleBySize(margins[4] + DMINIBAR_HEIGHT)
end

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

Just to chip in an opinion here (with the danger to be potentially totally irrelevant), instead of hiding/showing the status bar, there are a few pixels (default 7), to be used as a buffer for the extra pixels that a line could use.

DMINIBAR_HEIGHT = 7             -- Should be smaller than DMINIBAR_CONTAINER_HEIGHT 
DMINIBAR_CONTAINER_HEIGHT = 14 -- Larger means more padding at the bottom, at the risk of eating into the last line 

These or a some of them, could be used automatically (if so chosen from the user) and reclaimed if not needed anymore for the display of an extra line.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Tweaking the margins or the influence of the minibar will not solve the fact that there can always be pages with different height. With the DMINIBAR_HEIGHT=0, so no influence of it and only the pure margins we set (black borders are the allowed page rectangle for crengine, easier to see in 2-pages mode):

image

With the minibar enabled (but still no influence on margin, so drawn in the margin), and on this page, the publisher has set a different line-height for its things in italic:

image

Even if on this case, we feel like reducing a bit the bottom margin, so the page on the left can get one more line: well, some other pages may then still show such a blank line.

The idea of shifting the left page a bit down to balance its real used height into that black page rectangle would look odd in 2-pages mode (and possibly in 1 page mode, where you could possibly notice an extra blank space at top).

I remember I read somewhere that publishers of real books may solve the issue by padding (increasing) the line-height of the text lines, to distribute that blank space in between lines. But that looks not easy to do in crengine.

Edit: note that the above screenshots include some publisher page margins. No change in the overall issue with these additional margins removed:
image

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

I think it's typically more inter-paragraph/section spacing than line height, but of course that's not exclusive.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

I remember I read somewhere that publishers of real books may solve the issue by padding (increasing) the line-height of the text lines, to distribute that blank space in between lines.

This is true. I did some DTP for real books for a publisher and this was usually the way they solved problems like this.

I'm not thinking about shifting the left page, but more of changing the size of left page.
If the black rectangle is not visible, then it whouldn't be noticeable I think...

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

If I just shift down smaller pages by half the difference (not taking care of bits of the previous line shown and clipped, which I don't seem to be able to avoid...):
image

Even if the clipping was fixed, that does not look really acceptable to me.

I'm not thinking about shifting the left page, but more of changing the size of left page.

Well, you want to increase the height of the page because you think you can fit another line, good.
But either you just increase it globally (reduce the bottom margin), and there will always be pages that just didn't fit into that increase height that will leave a blank line.
On increse it under some conditions, and there will always be pages where that condition is just not met by a hair, 1 or 2 pixels, and you'll get a blank line anyway...

Here is when I allow fitting a line of the page if it overflows a bit, with some conditions:

             bool flgFit = currentHeight( line ) <= page_h;
+            if (!flgFit) {
+                int needed = currentHeight( line ) - page_h;
+                int fitting_pct = 100 - 100 * needed / line->getHeight();
+                if (fitting_pct > 80 && needed < gRootFontSize/3)
+                    flgFit = true;
+            }

image

With footer displayed:
image

With even more liberality:

+            if (!flgFit) {
+                int needed = currentHeight( line ) - page_h;
+                int fitting_pct = 100 - 100 * needed / line->getHeight();
+                if (fitting_pct > 50 && needed < gRootFontSize/2)
+                    flgFit = true;
+            }

image

image

Without the borders, and with that same liberal tweak, you can get either:

image

image

image

The issue is that a single line height less or more is noticable to the eye.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

On increse it under some conditions, and there will always be pages where that condition is just not met by a hair, 1 or 2 pixels, and you'll get a blank line anyway...

This is what I was thinking, but just using a, lets say, 3-4 pixel buffer that are empty already.
What I expect is that some lines might exceed the normal bottom of page by that amount.
If that is not enough for the extra line then we're talking about a very big difference and not just a couple of pixels...

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

That's your (and mine too) naive thinking that we can just do that and problem solved :)
(and I have a hard time accepting that this naive thinking does not work - so may be I'm wrong in proving it does not work... tell me where! :)
But I tried to show above that no, problem not solved.

A 3-4 pixel buffer is the same thing as decreasing the bottom margin by 3-4 px.

Say your lines are 16px tall. Allowing 4 pixels overflow: you have a line that can use them, good, use them. You have an other line on another page that would need 5px, you can't get it on that page: you have a 11px hole.
You're just left with some line 4px over the baseline, some others 11 or 12px under the baseline (so, a blank space of 12px, that is noticable - and some other lines possibly overflowing into the footer.) And there will always be a possible 16px difference between some pages.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

Well, I did warn you for my simplistic approach ;o)

I'll try to analyze my thinking a little more:
If our line is 16px you always have empty space at the bottom in the range of 0-15 pixels.
That's what we got now.
If we have a 4 pixels overflow, we get the bottom empty space in the range of 0-11px.
That's the improvement I'm thinking, knowing that its not a solution for no empty space at all.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

If we have a 4 pixels overflow, we get the bottom empty space in the range of 0-11px.

Relatively to the bottom margin line, yes. So, you'll have an empty space before if of 0 to 11px; And possibly some lines after that bottom margin overflowing it by 4px.
In absolute view, that still makes a possible16 px difference between where the last line is drawn on 2 different pages.

It's exactly the same as just pushing the bottom margin line 4px down (so, decreasing the bottom margin by 4px).

(I really have a hard time accepting this :) so please someone tell me if that thinking is right or wrong :)

Anyway, is this is right, you could see what you propose in an other way:
decrease the bottom margin from the value we get from the settings, by a shift depending on font size (and line-height?), and give that to crengine.
But you'll still get the possible 16px difference from one page to another.

@noembryo

This comment has been minimized.

Copy link
Author

commented Apr 7, 2019

It's exactly the same as just pushing the bottom margin line 4px down (so, decreasing the bottom margin by 4px).

True, you're right, but since I don't use a bottom margin, I didn't get it into consideration..
Personally I don't even have the extra pixel for the overflow ;o)
Used every available pixel for the page and to make the progress bar more usable..

Just thinking about the average tweaking user...

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