Repository navigation
ReaderFooter: document position is not used - #14076
Conversation
|
koreader/frontend/apps/reader/modules/readerfooter.lua Lines 599 to 603 in ac03e9c koreader/frontend/apps/reader/modules/readerfooter.lua Lines 970 to 977 in ac03e9c This ancient method is used in unit tests only. Currently the footer cannot be disabled in the UI. I think the method is not needed anymore. (I remember long ago the footer could be turned on/off in the bottom config menu, maybe this method was used there) |
|
It sounds useful, but if it seems redundant I guess it is. :-)) |
|
On the other hand, I tried to set |
|
Sounds good. |
| end | ||
| end, | ||
| page_progress = function(footer) | ||
| if footer.pageno then |
There was a problem hiding this comment.
Are we sure there is always a pageno ? (or/and a self.view.state.page to which it is set in :init() - edit: that you just removed :). Sure there won't be some event processing ordering that could have at some points pageno still being nil ?
Our footer stuff is so brittle, that I'm just uneasy removing these safeguards :)
But trusting you.
There was a problem hiding this comment.
That was the main point of investigation: to get the moment when pageno appears. I'm pretty sure it exists when calling the generators.
Also, look at the old code: this if is not a safeguard, it just switches to use position that definitely doesn't exist. The code path with position never happened.
There was a problem hiding this comment.
I removed the usage of self.view.state.page because the page is set in
koreader/frontend/apps/reader/modules/readerpaging.lua
Lines 145 to 147 in ac03e9c
and the footer is drawn later in the "ReaderReady" handler.
| if not self.initial_pageno then | ||
| self.initial_pageno = pageno | ||
| end | ||
| self.initial_pageno = self.initial_pageno or pageno |
There was a problem hiding this comment.
(I'm sometimes tempted to write as you just did - but the 3 lines make it clearer, and avoid an assignment, which must be more costly than just a if check, dunno if Luajit would optimize it.)
There was a problem hiding this comment.
I've read that this notation is recommended by the Lua creators, and it is optimized by the interpreter, the assignment is done when needed only.
| function ReaderFooter:onPosUpdate(pos, pageno) | ||
| self.position = pos | ||
| self.doc_height = self.ui.document.info.doc_height | ||
| if pageno then | ||
| self.pageno = pageno | ||
| if not self.initial_pageno then | ||
| self.initial_pageno = pageno | ||
| end | ||
| self.pages = self.ui.document:getPageCount() | ||
| self.ui.doc_settings:saveSetting("doc_pages", self.pages) -- for Book information | ||
| end | ||
| self:updateFooterPos() | ||
| self.pageno = pageno | ||
| self.initial_pageno = self.initial_pageno or pageno | ||
| self.pages = self.ui.document:getPageCount() | ||
| self.ui.doc_settings:saveSetting("doc_pages", self.pages) -- for Book information | ||
| self:onUpdateFooter() |
There was a problem hiding this comment.
Again, are we sure we always get pageno passed ?
I'm no longer confortable with all this, but I remember in the past I had been bothered by the different onPosUpdate/onPageUpdate (may be not in ReaderFooter, but elsewhere) and I'd rather not touch them if I don't have to :))
There was a problem hiding this comment.
The "PosUpdate" event is sent by ReaderRolling only, and it always passes the target page number as the second arg.
There was a problem hiding this comment.
I'm pleased to confirm that the above questions were the first/main questions that I have asked to myself and have investigated through the massive print-debugging.
There was a problem hiding this comment.
And, as I sadly mentioned in the first post, our footer architecture is not best optimized, some methods are called twice or more to draw the footer.
|
If the release is coming soon, we can postpone this. |
|
Depends a bit on the weather. It looks fairly outdoorsy the rest of the week. In any event, it won't be tomorrow. 
|
|
Avoid one more duplication. |
3 participants
The footer does not use "document position".
No need to have separate code paths for "page" and "scroll" view modes.
Removed duplicated code and code execution when building and showing the footer.
Closes #14065.
Despite of the above optimization, the footer text generator is still executed 3 times when opening an epub book, but looks like this cannot be fixed with the current architecture.
This change is