Statistics: fix getCurrentBookReadPages() when use_pagemap_for_stats#14426
Statistics: fix getCurrentBookReadPages() when use_pagemap_for_stats#14426poire-z wants to merge 1 commit into
Conversation
|
I have been testing while reading. The read pages are a little bit behind: I tried not scaling the reference pages and transforming them later using the pagemap table: And then later: There are some gaps for those synthetic pages not having a reference page (more screen pages than synthetic ones) but it seems fine: |
|
Any idea why the differences ? Why these 4 missing slots on the first screenshot ?
It feels this might be more expensive. Not sure it was really a good idea to store ref pages - the possible holes in Bookmap makes it really annoying and feel like a regression. |
To be honest I don' t know why. I see you added the db modified view in Lua code to use screen pages and for some reason the result of the scaled page gets behind. Maybe it has to do with there is no 1-1 mapping when using reference pages. I particularly don't care if the reference pages are stored in the book table since I use precalculated word count number from Calibre to measure how big a book is and % is enough to track. Although, I still find reference pages valuable to see consistently which page I am, how many pages left while reading... |
|
I read 3 pages of a book after skipping the first pages (table of contents, acknowledments...) and this is the result: This happens for nearly all books, because there are many pages at the beginning with not many characters. I modified the book file removing the first pages: Then removed db info for the book and the cache folder and repeat reading the same pages: It is consistent but as you keep reading it will vary because it is not linear. The approach I commented using the pagemap seems to be working fine. I actually started using the Book map to see which page I am in a chapter visually. |
|
Sorry guys, a random idea, but I think (I hope) it might help: And those won't go into actual statistics, because you didn't actually read them. |
|
Isn't that already what happens? If you consider the default of 5 s (?) too low you can always raise it. |
|
Yes, but I emphasized the visual aspect of it.
|
I see. This is a problem with our synthetic page map builder that does not know about page breaks :/ Does this happen also with real reference pages ? |
|
Yes, I initially tried with one having reference pages. |
You didn't show what's after, but if you didn't change it: |
|
I just added that conditional and yes, this trick it is not accurate. There are gaps from time to time. |
|
@hius07 : any thought about this mess (here, #14405 (comment)) about using ref/synthetic pages in statistics ? My gut feeling is that we were just fine/better with rendered pages - there can be so many problem with ref and synthetic pages vs. BookMap & PageBrowser. |
|
(Sorry, I don't use and don't know the book map well) I think we cannot calculate the reading time of a screen page based on the statistics of reference pages, |
|
But I imagine using reference pages in the statistics just as a changing of book formatting (font size etc.). |
BookMap interacts with (and use bits from) PageBrowser and works on screen pages.
It works - as shown here and around, with caveats: holes, ref pages possibly not spanning the whole book (try @jonnyl2 's provided test epub, I can't even make sense of what I see and the statistics...
That's what the "scaling" (of stats page to screen pages) is supposed to do. It will work somehow (with the caveats mentionned), but it's not super nice and doesn't track your reading (progress, with holes - and position, with possible shifts) well. |
|
I mean, let's assume the statistics base has some records for ref page [97]. The above screenshot shows ref page [97] partially filled, but from the ref pages statistics we don't know which screen page actually has been read. |
|
So ? (You described one of the issues.) |
|
Fill all of screen pages that are included into one ref page with the same bar. |
|
Again, from the ref pages statistics we don't know which screen page has been read, and scaling cannot help. |
|
That's what the scaling is already doing (this PR) - and what is shown in the screenshot just above. In the example above, it's even worse: page 97 spans... like 30 screen pages. There are so many cases where things can go worse in different ways:
Exactly ! And my wish we revert this, and go at just adapting in the statistics display the total ref/synth page count, reading ref/page percent, reading time per ref/synth page, etc... |
That's the question to the book publisher. I hope there will be no issue with accurate synthetic pages. In any case, I do not fight for this feature, ready to revert it. |
Except it gave us a book and is gone and can't be reached :) and we have to do with it, and give our users something usable if we can.
That's what they'll see in the footer & co.
Possibly less (because they should span the whole book - unlike ref pages that can stop), but you may still have some if some screen pages are filled with pictures and no text, and you may have a synth page spanning 20 screen pages, while on other pages with text, you'll have 2 synth pages per screen pages... If we go at storing screen pages in statistics, we then only have to deal with numbers shown in KVPage - we can do what we can, but if we fail, they just won't be that accurate, they should look coherent anyway - unlike what we see in BookMap and PageBrowser where it does hurt your eyes. |
|
No longer necessary after we reverted the cause in #14435. |
|
Mentionning that here, because it's really noticable in the screenshot above at #14426 (comment) : In crengine's // We loop thru all text nodes, whether visible or not (as visiblity can be changed
// with styles, and we want this to be stable)
while ( xp.nextText() ) {
// But skip text from elements which would by default be hidden (<style>, <script>,
// some FB2 elements...), mostly so that a huge <style> at top of document doesn't
// generate many synthetic pages that would end up all on the first screen page.
const css_elem_def_props_t * ntype = xp.getNode()->getParentNode()->getElementTypePtr();
if ( ntype && ntype->display == css_d_none ) {
continue;
}
lString32 txt = xp.getText();
[...]I still don't want to have to look at the publisher styles for any explicite page-break, that we could account for by emitting a new page number - because users can disable or tweak publisher styles, and I really want synthetic page numbers to be stable whatever the styles change. |
|
Before emitting a new synthetic page number when we meet a new DocFragment: After: After, zoomed on some pages to show them in margins: @dmalinovsky @jospalau @hius07 (and others who like synthetic page numbers): |
|
Doesn’t matter honestly, as long as this number is consistent. Whatever makes more sense for you. |
|
Same. I need reference pages to have consistent "page" numbers in all my books but for statistics I don't mind. Actually, I put attention to screen pages for chapter pages when reading. |
|
No idea yet, may be I screwed up the counting while changing the way we do it (needed to account for DocFragments)... |
|
No such issue with the previous code :/ |
|
OK, obviously I didn't test if with paragraphs larger than the chars_per_page :/ Still not heavily tested, but this reads better: --- a/crengine/src/lvtinydom.cpp
+++ b/crengine/src/lvtinydom.cpp
@@ -20999,14 +20999,17 @@ void ldomDocument::buildSyntheticPageMap( int chars_per_synthetic_page )
else { // isText()
lString32 text = n->getText().trim();
int len = text.length();
+ int offset = 0;
+printf(" %d %d %s\n", len, count, UnicodeToUtf8(ldomXPointer(n, offset).toString()).c_str());
while (len > count) {
- int offset = count;
+ offset += count;
len -= count;
count = chars_per_synthetic_page;
page_num++;
lString32 title;
title.appendDecimal(page_num);
m_pagemap.addPage(title, ldomXPointer(n, offset), lString32::empty_str);
+printf("%03d %d %d %s\n", page_num, len, count, UnicodeToUtf8(ldomXPointer(n, offset).toString()).c_str());
}
count -= len;
}Before (with only these printf()): After: |
Fix #14388 (comment).
@jospalau : can you check this version too ? (same SQL, just proper if/else branches, but just want to double check it is fine).
This change is