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 some footnotes and full status bar issues #4268

Merged
merged 3 commits into from Oct 14, 2018

Conversation

Projects
None yet
4 participants
@poire-z
Contributor

poire-z commented Oct 12, 2018

See individual commits messages.
Fix some issues reported at #4261 (comment)

A few notes:

I moved the self.ui.document:setStatusLineProp(status_line) which is the call that enables the status bar in crengine from ReaderFooter to ReaderRolling, because it's more logical, and the ReaderFooter:onSetStatusLine is set to a no-op when footer is totally disabled.

I just discovered that Scroll mode and Full status bar don't work well together (no full status bar displayed when scroll mode is enabled). This looks like it's not supported by crengine itself (I have no intent to fix this combination of 2 rarely used options :)

I fixed: when switching Progress bar from 'full' to 'mini', show the mini bar again, with the side effect that the mini bar is now hidden when the full status bar is shown - including when loading a document when one have full status bar set. Previously, the mini bar was still shown with the full status bar !
So, people who liked to have both will have to tap on the footer to make the mini bar appear.
This behaviour looks more correct with the fact that full|mini is a toggle, so it's one or the other.
But I guess some people enjoyed having them both (because the top bar doesn't give much info).
We'll see if something needs to be done to have both...

poire-z added some commits Oct 12, 2018

Fix footnotes and markers with full status bar
Take full/top status bar's height into account when
computing coordinates.
Also wrap getPageMargins() in CreDocument, so we
don't use cre.cpp _document directly anywhere else.
Footnotes: fix current link being unhighlight in some cases
When a link is covered by the footnote widget, we highlight it
again when closing the footnote, and schedule an unhighlight
0.5s later.
When we tap on another footnote link, this was happening too
but the unhighlight unhighlighted all, including the new
footnote link.
This disable the former when the latter happens.
@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

OK, some unit test fails because of my behaviour change:

it("should toggle between full and min progress bar for cre documents", function()
local sample_txt = "spec/front/unit/data/sample.txt"
local readerui = ReaderUI:new{
document = DocumentRegistry:openDocument(sample_txt),
}
local footer = readerui.view.footer
footer:applyFooterMode(0)
assert.is.same(0, footer.mode)
assert.falsy(readerui.view.footer_visible)
readerui.view.footer:onSetStatusLine(1)
assert.is.same(0, footer.mode)
assert.falsy(readerui.view.footer_visible)
footer.mode = 1
readerui.view.footer:onSetStatusLine(1)
assert.is.same(1, footer.mode)
assert.truthy(readerui.view.footer_visible)
readerui.view.footer:onSetStatusLine(0)
assert.is.same(0, footer.mode)
assert.falsy(readerui.view.footer_visible)
end)

Line 629 now gets what line 634 gets.
So, can someone confirm which behaviour is preferable :)
Shoud toggle from Full to mini show the mini bar - or let it hidden?

@AlanSP1

This comment has been minimized.

AlanSP1 commented Oct 12, 2018

Just to mention, I personally prefer mini (lower) status bar, full is not that useful to me, but I guess that there's some people who prefer having full (upper) bar, because they don't have same info.

Full has book name info, mini doesn't. I think everything else is on mini's side, i.e. mini has much more info and is more configurable.

@Frenzie

Perhaps some of this predates G_reader_settings? In any case, nothing jumps out at me. Didn't test though. ;-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

Just to mention, I personally prefer mini

Me too, and no change about that! :)
But currently, people who set full via the bottom config toggle gets the top status bar and the action of toggling hide the footer. This looks like coherent behaviour.
But if the same people who just did that, now restart koreader or change book and back: they get both the top status bar AND the bottom mini bar, and they could go on with that and be happy with it.
Then, currently, if you then toggle again back to mini, the top status bar is removed, but you don't see yet the bottom bar: you need to tap on the bottom to make it shown.

^ That is the behaviour I have on my Kobo with current nightly and on the emulator. May be it's messed up because of some other personal setting.

Or may be these people need additionaly to uncheck everything in the Status> submenu, so the bottom status bar is naked and not displayed? So the current twisted behaviour provides an additional feature: both status bar, that my fix would remove?

@Frenzie : so, in the train :) you could just play with these and see if the current behaviour looks as messy as I found it - and see if the behaviour this PR provides feels better or the right one.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

pinging @gerhaher , who uses the top status bar, for his opinion: do you have the bottom bar too? Do you like it? If not, did you make something specific to not have it?

@gerhaher

This comment has been minimized.

gerhaher commented Oct 12, 2018

I use both, yes. But the bottom has more info so that is much more important. The top bar is more like "icing on the cake".

I just want that both are shown when I open a new book. Without have to do anything. This is how it works now after I've manually changed the default.persistent.lua. :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

Well, then I guess we'd better have 3 options that work all as expected when set, toggled, set from default on new book:
image

Easy to do, pushing a commit with it (I'll remove it or squash it into 3rd commit depending on what we decide).

@poire-z poire-z force-pushed the poire-z:various_fixes branch from ab106eb to f0fdf25 Oct 12, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 13, 2018

Or we can do simpler, without that 3rd both option: just have ReaderFooter ignore on book loading that setStatusLine event and just start with its last used value. So that full|mini will just hit ReaderRolling to toggle the top status bar.
With just may be, when toggled from full to mini (not when loading a book), force a show of the mini bar.

@poire-z poire-z force-pushed the poire-z:various_fixes branch from f0fdf25 to 59be314 Oct 14, 2018

Fix default not being used for View mode and Progress bar
Default setting set by holding on the bottom config buttons
for "View mode" (scroll/page) and "Progress bar" (full/mini)
were not used.

Also, when switching Progress bar from 'full' to 'mini',
show the mini bar again.

@poire-z poire-z force-pushed the poire-z:various_fixes branch from 59be314 to a200b19 Oct 14, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 14, 2018

OK, removed both, and did just as in my last message.
Mostly because both would just make one thing consistent, but there are a few others that are still not (like footer status being global and non per-book, which is fine, but its visibility would depend on the full|mini|both setting, or not with just full|mini).
And it then should be named Status bar: top|bottom|both, and the Status bar menu item: Bottom status bar.
So quite a few things to fix if we wanted to make all that coherent (and I don't care much :).
But mostly for aesthetic reasons, and not have a 3-items toggle in that last bottom config panel :)

So at the end, this just make Hold/default works, and shows back the mini bar when toggling from full to mini.

@poire-z poire-z merged commit fa0117b into koreader:master Oct 14, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:various_fixes branch Oct 14, 2018

poire-z added a commit to poire-z/koreader that referenced this pull request Oct 22, 2018

Cleanup onSetStatusLine()
Follow up to fa0117b (koreader#4268), to make things a bit clearer:
Only ReaderRolling get the 'SetStatusLine' event, and tells
crengine about the change, and then send the 'UpdatePos' event.
ReaderFooter now just gets a :setVisible() method.

Now, all the code that calls a self.ui.document:set* method, that
most probably triggers a full re-rendering by crengine, do signal
'UpdatePos' immediately after. This event signals that all page
number, pages count, positions... are no more valid and must be
queried or computed again.
This could also be used if we ever want to cache Page Links or
Screen Boxes: this event will have us dropped these caches.

poire-z added a commit to poire-z/koreader that referenced this pull request Oct 22, 2018

Cleanup onSetStatusLine()
Follow up to fa0117b (koreader#4268), to make things a bit clearer:
Only ReaderRolling get the 'SetStatusLine' event, and tells
crengine about the change, and then send the 'UpdatePos' event.
ReaderFooter now just gets a :setVisible() method.

Now, all the code that calls a self.ui.document:set* method, that
most probably triggers a full re-rendering by crengine, do signal
'UpdatePos' immediately after. This event signals that all page
number, pages count, positions... are no more valid and must be
queried or computed again.
This could also be used if we ever want to cache Page Links or
Screen Boxes: this event will have us dropped these caches.

poire-z added a commit that referenced this pull request Oct 23, 2018

Cleanup onSetStatusLine()
Follow up to fa0117b (#4268), to make things a bit clearer:
Only ReaderRolling get the 'SetStatusLine' event, and tells
crengine about the change, and then send the 'UpdatePos' event.
ReaderFooter now just gets a :setVisible() method.

Now, all the code that calls a self.ui.document:set* method, that
most probably triggers a full re-rendering by crengine, do signal
'UpdatePos' immediately after. This event signals that all page
number, pages count, positions... are no more valid and must be
queried or computed again.
This could also be used if we ever want to cache Page Links or
Screen Boxes: this event will have us dropped these caches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment