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

Fix: Status bar hides text with minimal bottom margin #5135

Merged
merged 3 commits into from Jul 22, 2019

Conversation

@robert00s
Copy link
Contributor

commented Jul 22, 2019

With very narrow bottom margin text is sometimes hidden by status bar.
Before:
obraz
After:
obraz

@robert00s robert00s requested review from Frenzie and poire-z Jul 22, 2019

@@ -506,7 +506,7 @@ function ReaderTypeset:onSetPageMargins(margins, refresh_callback)
if self.view.footer.has_no_mode or self.view.footer.reclaim_height then
bottom = Screen:scaleBySize(margins[4])
else
bottom = Screen:scaleBySize(margins[4] + DMINIBAR_HEIGHT)
bottom = Screen:scaleBySize(margins[4] + self.view.footer:getHeight())

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Jul 22, 2019

Member

Shouldn't the self.view.footer:getHeight() be moved outside of the scaleBySize call? Since I assume it's returning an already-scaled container size in pixels, right?

This comment has been minimized.

Copy link
@Frenzie

Frenzie Jul 22, 2019

Member

Definitely, yeah.

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 22, 2019

Contributor

Indeed, it's made out from height = Screen:scaleBySize(DMINIBAR_CONTAINER_HEIGHT).
After this is fixed, lgtm.

@Frenzie Frenzie added this to the 2019.08 milestone Jul 22, 2019

@Frenzie Frenzie added the UX label Jul 22, 2019

@Frenzie
Copy link
Member

left a comment

lgtm

@Frenzie Frenzie merged commit 69ee8c4 into koreader:master Jul 22, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
return self.footer_content:getSize().h
else
return 0
end

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 22, 2019

Contributor

Why was that needed for? (And why now and never before?)

I'm just worried that if it's an ordering issue (some other module calling this before ReaderFooter is set-up), we may end up with a different bottom margin, which may trigger after first load/rendering, on the next painting, a second rendering, or something - or when loading from cache if we start with this being 0 and a smaller bottom margin, and end up with this being correctly non-zero and then a larger bottom margin.)

This comment has been minimized.

Copy link
@robert00s

robert00s Jul 22, 2019

Author Contributor

I need to use it because unit test fails:
https://circleci.com/gh/koreader/koreader/3113#tests/containers/1

frontend/apps/reader/modules/readerfooter.lua:298: attempt to index field 'footer_content' (a nil value)

stack traceback:
	frontend/apps/reader/modules/readerfooter.lua:298: in function 'getHeight'
	frontend/apps/reader/modules/readertypeset.lua:509: in function 'onSetPageMargins'
	frontend/apps/reader/modules/readertypeset.lua:92: in function 'handleEvent'
	frontend/ui/widget/container/widgetcontainer.lua:88: in function 'propagateEvent'
	frontend/ui/widget/container/widgetcontainer.lua:106: in function 'handleEvent'
	frontend/apps/reader/readerui.lua:385: in function 'init'
	frontend/ui/widget/widget.lua:48: in function 'new'
	spec/front/unit/readerfooter_spec.lua:372: in function <spec/front/unit/readerfooter_spec.lua:360>

[  ERROR   ] spec/front/unit/readerfooter_spec.lua @ 360: Readerfooter module should not schedule auto refresh time task if footer is disabled (149.12 ms)

This comment has been minimized.

Copy link
@robert00s

robert00s Jul 22, 2019

Author Contributor

I can leave function ReaderFooter:getHeight() as was before and change function ReaderTypeset:onSetPageMargins() (adding self.view.footer.settings.disabled in if)

function ReaderTypeset:onSetPageMargins(margins, refresh_callback)
    local left = Screen:scaleBySize(margins[1])
    local top = Screen:scaleBySize(margins[2])
    local right = Screen:scaleBySize(margins[3])
    local bottom
    if self.view.footer.has_no_mode or self.view.footer.reclaim_height or self.view.footer.settings.disabled then
        bottom = Screen:scaleBySize(margins[4])
    else
        bottom = Screen:scaleBySize(margins[4]) + self.view.footer:getHeight()
    end
    self.ui.document:setPageMargins(left, top, right, bottom)
    self.ui:handleEvent(Event:new("UpdatePos"))
    if refresh_callback then
        -- Show a toast on set, with the unscaled & scaled values
        UIManager:show(InfoMessage:new{
            text = T(_([[
    Margins set to:

    horizontal: %1 (%2px)
    top: %3 (%4px)
    bottom: %5 (%6px)

    Tap to dismiss.]]),
            margins[1], left, margins[2], top, margins[4], bottom),
            dismiss_callback = refresh_callback,
        })
    end
    return true
end

This comment has been minimized.

Copy link
@poire-z

poire-z Jul 22, 2019

Contributor

I need to use it because unit test fails

OK, so it's just because that function was now called in a rarely tested case and it failed.
So, I guess your change is fine.

I can leave function ReaderFooter:getHeight() as was before and change function ReaderTypeset:onSetPageMargins() (adding self.view.footer.settings.disabled in if)

Well, no, it's fine where you put it. Because I'm sure that the other places where I use footer:getHeight() would then fail too if I tested them with footer disabled :) (Even if it looks to me there's no way to set disable=false via the UI, so that unit test is testing something that's never happening/used ?)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I upgraded today to a KOReader with this PR merged, and I had a bit of a hard time dealing with the huge bottom margin I got because of this :)
There's nothing wrong with this PR, looking at the code and computed sizes, everything seems right.

It's just that personally, wanting to have the top and bottom margins balanced (in the bottom menu), and visually balanced on the screen to my eyes, it was a bit tedious.

I've been checking this with a Wikipedia EPUB and in-page footnotes enabled (crengine adjust the footnotes position so they are stuck to the bottom content area (with normal books, it's less noticable cause you only adjust top/bottom margins to try to fit one more line).

  • with Reclaim bar height from bottom margin enabled, the bottom margin feels a bit short, and there is the risk to have the last line eaten by the footer
  • with Reclaim bar height from bottom margin disabled, the bottom margin feels too tall.

I finally solved this to my liking with a personal patch:

--- orig_koreader/frontend/apps/reader/modules/readertypeset.lua
+++ koreader/frontend/apps/reader/modules/readertypeset.lua
@@ -506,7 +506,16 @@
     if self.view.footer.has_no_mode or self.view.footer.reclaim_height then
         bottom = Screen:scaleBySize(margins[4])
     else
-        bottom = Screen:scaleBySize(margins[4]) + self.view.footer:getHeight()
+        -- bottom = Screen:scaleBySize(margins[4]) + self.view.footer:getHeight()
+        -- Somehow, the previous buggy behaviour felt nicer:
+        --   bottom = Screen:scaleBySize(margins[4] + DMINIBAR_HEIGHT)
+        -- but last line gets truncated with small margins.
+        -- So, tweak that a bit
+        local footer_height = self.view.footer:getHeight()
+        bottom = Screen:scaleBySize(margins[4]) + Math.round(footer_height / 2)
+        if bottom < footer_height then
+            bottom = footer_height
+        end
     end
     self.ui.document:setPageMargins(left, top, right, bottom)
     self.ui:handleEvent(Event:new("UpdatePos"))

(With my screen/SPI, Screen:scaleBySize(DMINIBAR_HEIGHT) == self.view.footer:getHeight() )

So, I'm personally fine.

But I wonder if other people feel like me, that the default equal top and bottom margin values, with either toggle state of Reclaim bar height from bottom margin, don't give a really nice view by default?

And pinging @NiLuJe if the reason for adding that Reclaim bar height from bottom margin was just to somehow decrease the bottom margin appearing value while keeping the buttonprogress values somehow balanced? And if it is really necessary, or if there is some other computation rules for the final computed bottom margin that would appear generically nice without the need for that toggle?
(Hope I'm not too confusing, hard to explain personnal feellings :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

I generally don't use the minibar (if I need an idea of where I am in the book, I have a corner tap set to show the skim widget, which roughly matches my habit of checking the bar graph in the native KePub reader), and I like extremely narrow margins, so, my use-case is generally L=R=T=B at <= 5 (+/- 1 for T/B in the rare case it's unbalanced enough to actively bother me).

Without Reclaim bar height from bottom margin, a setup like that wasn't possible ;).

(EDIT: I mean, in term of actually having a narrow bottom margin, I'm not OCD enough to care that everything's equal to an arbitrary value, even if that turned out to be the case with my defaults ;)).

I haven't had a case of stuff being cut-off at the bottom in quite a while, if there's no space (or at least if CRe assumes there's not enough space ;p), the full line dutifully gets chucked to the next screen.
(That's with fairly narrow line-spacing, and a patched Bookerly with virtually no interline, which should bias towards clipping, and not towards chucking it to the next screen).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Random example with what I'm currently reading:

First page of a chapter, not enough space for the final line (... apparently ^^):
ko-1

Next page, everything back in order:
ko-2

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

That falls in the "bearable" slight unbalance here, with the slightly larger bottom margin.

(Everything is at my defaults of 5 here).

It actually helps when there's smallcaps or extra paragraph line-spacing or other trickery, like inlined blocks or tables.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Sidenote: I think you meant "disabled" in your second bullet point, right? ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

^ right :) corrected.
OK about your setup and the need for Reclaim... (which would not be needed as we would know the minibar is disabled - but we don't want to trigger a re-rendering when temporarily enabling it).

Care to check how it would look for you with the minibar enabled (if that don't mess your settings), and if you feel or not like I did?
(The unbalance is actually less noticable/more beatable with regular book like on your screenshots - more bothering when you have in-page footnotes that are shoved down.)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

If I just toggle the minibar, it clips, as expected:

ko-3

If I disable Reclaim... I end up with a perfectly fucked up setup where I don't have enough space for a final line above the minibar by something like 5 to 10px (in a somewhat similar fashion as to my first example, where I'd need to go down to a bottom margin of 2 (5px) to get the line to show up).

(EDIT: Well, not exactly, here, I'm not even sure I could fit a line without playing with font-sizes or line-spacing).

ko-4

ko-5

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

FWIW, my usual goal is to get 37 lines to show up (one way or another ;p) when there's no extra breaks, as that's what my KePub setup achieves in Nickel ;).

That usually only takes minimal nudging with line-spacing, or a half font size. And +/- 1 (or rarely 2) in T/B margins.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

If you keep the minibar, and disable Reclaim, I'm not sure there's a silver bullet here, except possibly ensuring the minibar container height is not fixed, but matches the exact same height as a line of (body) text, which I'm not sure we can do, but I imagine would make both CRe and the user's life easier as far as dealing with bottom whitespace is concerned ^^.

(I imagine the fact that it's just not quite the same height as a line of text here makes dealing with it impossible without more drastic changes to the text layout (i.e., font size, line spacing)).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

^ I have no issue dealing with that with just decreasing bottom margin, and hiting Sync T/B when done to get things balanced. I never target a specific number of lines, I start with a prefered font size for my eyes at this time of the day, and never tweak interline space :)

But that's not really my issue, and it clouds it a bit :)
My issue is that when buttonprogress are balanced, the area available to crengine is unbalanced, and you can best see it on a Wikipedia EPUB and enabling in-page footnotes (Styletweaks > Misc): then, the extra height will not be some blank at the bottom, but it will be put (by crengine) between the main text and the footnotes text (in yellow below).
image
To me, the red bottom margin feels taller than the blue top margin - but they are equal, it is perfectly logically right code wise: we put the bottom margin above the footer height, but as the footer is sparse, it feels like the bottom margin is taller.

With Reclaim... enabled, it feels smaller:
image

That's with both T/B balanced in the bottom menu:
image

My issue is that none of the 2 screenshots above looks nice, while something in between may (that somehow the logically wrong previous code did, as it neved bothered me before :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

That's because it's only actually balanced with reclaim enabled. Since without it, what we feed CRe is effectively B + MiniBarContainerHeight, which is obviously larger than T if B == T ;).

(i.e., while I essentially agree with you on the effective results, the maths do make sense).

(Unless you meant the red squiggle NOT to include the minibar, in which case, yeah, it all comes down the the height of the minibar container vs. the height of an actual line of text, and the fact that there's padding in said minibar visually skewing things up a bit more ;)).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I guess you could also enable Reclaim, and fine-tune a larger bottom margin so that it ends up closer the edge of the bar's content, instead of the bar's container, which I imagine would be close to what happened before this PR ;).

That definitely means T != B, though, which I personally have no issue with ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

TL;DR: I'm generally not a fan of the behavior w/ the minibar enabled & Reclaim disabled, with or without this PR, but I'm not sure there's a way to make it any better for everyone.

With reclaim enabled, you have a bit more control over how you want to make it look, though, since you're no longer tributary to a magical (and unknown) hard-coded value that gets appended to your margins.
But having the minibar enabled certainly makes this process slightly longer to get "right", whatever that may be for you ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I guess what I said in #5135 (comment) is similar is spirit as to what your tweak does:

For instance (haven't actually checked, but I doubt it's exactly the case), If we knew that the container's padding (top + bottom) was exactly as tall as the bar's content, we could use three quarters of self.view.footer:getHeight() (i.e., effectively discarding the top padding of the container).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I'm not familiar with how the widget is built, but we might actually know the padding size exactly, in which case we can just use that (as-is or / 2 if it's top + bottom) + scaleBySize(DMINIBAR_HEIGHT) (since that last bit should be only the actual content's height, right)?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

I'm not familiar with how the widget is built, but we might actually know the padding size exactly

I looked at how the footer is build, and there's no real top padding (there's some bottom padding). So, it might just be the tip of the ascender of the font used for the text, which is not much.
And you hardly see any padding when you set the bottom margin to 0 (with Reclaim disabled): the book's text goes near the footer, and there's no feeling of too much or too litlle padding above the footer: it's just right.
So, nothing to tweak on the footer container/content/padding code I think.

That's because it's only actually balanced with reclaim enabled. Since without it, what we feed CRe is effectively B + MiniBarContainerHeight, which is obviously larger than T if B == T ;).
(i.e., while I essentially agree with you on the effective results, the maths do make sense).

Yes, I get that :) all is fine and logical.

And there are multiple ways to get back the visual balance, and users will be able to.
It's just a bit sad that it's no more balanced by default, and that you'll have to rebalance it everytime you change font size, or decide for some other margins.
(And actually, in a regular book without footnotes, with Reclaim disabled, one will strive to try to get that additional line in that space and balance that again with the Sync T/B, as it will re-unbalance it.)

while I essentially agree with you on the effective results

phew, thanks :) I'm not that crazy :)

what we feed CRe is effectively B + MiniBarContainerHeight, which is obviously larger than T if B == T

What I guess I'd like to find is a function f(MiniBarContainerHeight) or f(MiniBarContainerHeight, B) that, when T=B (logically balanced), would make it visually/optically balanced.

My creB = Max(B + MiniBarContainerHeight/2, MiniBarContainerHeight) function works for me, but there's no science behind it :) and it may not work well with other font sizes/families or interline spaces, or for people who only have the progress bar (which is smaller than footer text) in the footer, or only text with no progress bar, or...

With reclaim enabled, you have a bit more control over how you want to make it look, though, since you're no longer tributary to a magical (and unknown) hard-coded value that gets appended to your margins.

Yep, but that's that magical thing that would make it optically right in every cases I'm looking for :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Okay, I did some more testing.

Wanted to begin by confirming that self.view.footer:getHeight() wasn't feeding us bogus data.
Good news, it doesn't: B: 12 + 37 = 49

(12px is my '5' margin setting after scaling, 37px is the bar, confirmed with my trusty GIMP ruler and my third screenshot ;)).

So, at least that's right, and we're just leaving CRe to deal with how much space we're telling it is available.

Which leads me to the next experiment: what if we insisted on the fact that bottom margin means the margin from the edge of the screen, always, instead of just from the edge of the minibar.

That's easy enough:

diff --git a/frontend/apps/reader/modules/readertypeset.lua b/frontend/apps/reader/modules/readertypeset.lua
index ea6cbb8c..07cfb54e 100644
--- a/frontend/apps/reader/modules/readertypeset.lua
+++ b/frontend/apps/reader/modules/readertypeset.lua
@@ -505,8 +505,10 @@ function ReaderTypeset:onSetPageMargins(margins, refresh_callback)
     local bottom
     if self.view.footer.has_no_mode or self.view.footer.reclaim_height then
         bottom = Screen:scaleBySize(margins[4])
+        print("B: " .. bottom)
     else
-        bottom = Screen:scaleBySize(margins[4]) + self.view.footer:getHeight()
+        bottom = math.max(Screen:scaleBySize(margins[4]), self.view.footer:getHeight())
+        print("B: " .. Screen:scaleBySize(margins[4]) .. " + " .. self.view.footer:getHeight() .. " = " .. bottom)
     end
     self.ui.document:setPageMargins(left, top, right, bottom)
     self.ui:handleEvent(Event:new("UpdatePos"))

Which effectively leads to B: 12 + 37 = 37 since my bottom margins are so narrow.

I'm not sure how that would affect your example, but, it doesn't change anything on mine (perfect match with my two last screenshots) ;).

(Except for minor one pixel shifts in the position of the notches in the minibar progress bar).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

I'm not sure how that would affect your example, but, it doesn't change anything on mine

Well :) you just made Reclaim enabled and disabled identical then :) with just the additional security of a minimal margin = the footer height, when Reclaim disabled.
Which would then make the bottom margin felt smaller to me :|

Except for minor one pixel shifts in the position of the notches in the minibar progress bar

Horizontal (when comparing bit by bit your screenshots?) or vertical shifts? If vertical, it must be some non-integer values somewhere, that would need to be math.round()ed.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Horizontal (when comparing bit by bit your screenshots?) or vertical shifts? If vertical, it must be some non-integer values somewhere, that would need to be math.round()ed.

Horizontal, yes ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

To recap:

AFAICT, this PR does the "right" thing, if we assume the bottom margin needs to start at the edge of "content", and not at the edge of the screen.

What we're left with is the usual thing: you just cannot assume that numerically balanced T/B margins will yield visually balanced whitespace. That's never been the case, and it never will be. That's not limited to CRe. A glyph's bounding box (and, in turn, a line's bb) may include a significant amount of whitespace, and that amount may be significantly different at the bottom than at the top. The renderer doesn't really know exactly how much.
The fact that the minibar itself has varying amount of whitespace (you were right, the tallest bit is the text) muddies the waters even more.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

AFAICT, this PR does the "right" thing, if we assume the bottom margin needs to start at the edge of "content", and not at the edge of the screen.

Yes it does, it does what we made it do - but is what we made it do the best? :)

I still feel creB = Max(B + MiniBarContainerHeight/2, MiniBarContainerHeight) gives a better visually balanced result in the many cases I checked. But it's a "magic" thingy. But we never say explicitely that (without Reclaim enabled), we do the selected margin from the footer's top :)
But ok, let's wait for/if more feeback.

matches my habit of checking the bar graph in the native KePub reader

Just curious, as you seem to use both Nickel and KOReader: why/when do you decide to use one or the other?

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

But we never say explicitely that (without Reclaim enabled), we do the selected margin from the footer's top :)

I don't think most people would intuitively think of the footer as an overlay.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I mainly used Nickel on the H2O, because the limiting factor was the screen itself, and I was used to it ;p.

On the Forma, we end up being significantly snappier, so, I tend to do most of my reading there. I do drop back in Nickel from time to time, mainly to sync read status when dropping new content via Calibre ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@Frenzie

I don't think most people would intuitively think of the footer as an overlay.

i.e., you think the current behavior (margins start from the edge of the content (minibar), and not the screen) makes more sense?

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I do think this PR and the current no-reclaim behavior make sense and are technically "right".

That it appears to end up looking like crap most of the time when the minibar is shown is unfortunately somewhat irrelevant, as everything is doing the best they can given what they're told (i.e., the only fix is to tweak the margins to your heart's content, given your own font/interline settings) ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@NiLuJe

i.e., you think the current behavior (margins start from the edge of the content (minibar), and not the screen) makes more sense?

Yes. So that the margin starts above this line:

Screenshot_2019-08-03_18-16-03

In actual practice it'll seldom make an actual difference due to bounding boxes, line height, etc. It's just the theoretically right behavior.

Unfortunately we don't do this yet on paged media, where it actually matters a lot more. ;-)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

@Frenzie: Agreed, the exceedingly few times I opened up a PDF, it was annoying as hell ;). Especially since I'd possibly tend to keep the minibar shown on a PDF, strangely enough.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I use show all at once (which imo is always preferable) so that it's easily toggled on & off. It's not really annoying in CBZ/DjVu/PDF though, at least if you use content width + scroll mode like I do. The overlap takes care of that.

But I recognize that having gotten used to quickly toggling the statusbar every so often is a workaround for a problem that shouldn't exist.

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