Updates 2020.08-1 #157

virxkane · 2020-08-29T17:47:51Z

Fixed fb2 coverpage drawing in scroll mode (continuous) with enhanced render.
Android: use fast (not precise) mode for functions LVDocView::getBookmark() only for page turn & text scroll.
Optimized calculation of average animation duration.
Android: fixed scrolling of text around start and end of a book in scrolling mode.
Improved methods for selecting next/previous sentence when using "***" paragraph separators. In fact, this is a continuation of PR Restored sentence selection in TTS that broken in 3.2.39 resulting in… #144, i.e. restoration of old functionality.
Legacy rendering mode: display block elements that are inside inline elements as block elements.
Trying to update the structure of a database that has been modified by some kind of uncoordinated fork of the program.
Forced update the DOM level in database from previous newest to latest newest.

Updates from KOReader:

New HTML parser, libRu and FB2 tweaks New HTML parser, libRu and FB2 tweaks koreader/crengine#370

@poire-z

… render. With corrective patch @poire-z.

The content following the 'display:run-in' footnote number is usually a P rendered erm_final, and this merging of them into a container erm_final works quite well. But when the content is not a P but some more complex erm_block structure (like poem>stanza>v or cite>p), ensuring this mergins would make that structure all erm_inline, losing the formatting. So don't do that, and let a blank line after the footnote number happen, which is less worse than losing formatting.

If the font provides the 'tabular-nums' (tnum) OpenType feature, we'll get more properly aligned starts of text after the numbers.

Some of these can still be found in old documents or badly converted ones, and most fonts have no glyph for them (and if they do, it's a small symbol with no meaning for the reader).

Strangely, when floats are involved, a HR behaves differently than a regular DIV and adjust to the available space. Nothing mentionned about that in the specs, so try to handle them as Firefox does.

Avoid crash when called while styles not yet set or reset. Show the rootnode as <?RootNode?>.

So DOM writers can distinguish from and handle these differently.

Accept <!DOCTYPE html> which may not have any <html>, <head> and <body> tags (they are then implicit), and any other kind of <!DOCTYPE ... HTML ...> which are obviously HTML, no matter how broken they are.

Avoid the first (wrong) rendering from being different than next (good) ones, because it was parsed as PRE and the node was substituted to a DIV when closed. Handle Lib.ru books in a more correct DOM building way.

Follow most rules from the HTML specs. Add a new DOM_VERSION_CURRENT 20200824 so newly opened HTML and CHM files use the new parser (which might give a different DOM tree than the previous one, which will still be used for previously opened book to preserve XPointers). fb2def.h: add a few more tags mentionned in the specs.

According to the specs, non-table elements met while building a table (before we meet a TD/TH), should be moved outside the table and added as the previous sibling of the table. Previously, these elements were kept there, and ignored by the table rendering code.

<?xml?> and the like are parsed and fed to the DOM writers, but let's ignore them and not include them in the DOM tree.

…mark() **only** for page turn & text scroll. Also don't call LVDocView::getPageDocumentRange() in fast mode.

…rolling mode.

…" paragraph separators. In fact, this is a continuation of PR #144, i.e. restoration of old functionality.

…elements as block elements.

…y some kind of inconsistent fork of the program.

…t newest. Allowed to changing the DOM level and block rendering flags in options dialog for CHM files.

poire-z · 2020-08-29T18:08:12Z

A note about // Forced update DOM version from previous latest (20200223) to current (20200824). :
The new HTML parser can change the DOM tree from the previous one - and it's not something we can migrate. So, past highlights/bookmarks (in HTML, including lib.ru books, and CHM only) might not be found when the DOM is changed from 20200223 to 20200824 (no impact on other formats as they use the unchanged XML parser). koreader/crengine#243 (comment) . That's our curse :/
(I did prevent that upgrade in the first chunk of https://github.com/koreader/koreader/pull/6560/files .)

virxkane · 2020-08-29T18:19:40Z

@poire-z The one public released version with DOM 20200223 is 3.2.45 released few days ago. So I think this is not critical change, but fixes rendering of strange chm files.

poire-z · 2020-08-29T18:27:33Z

Oh, right :) so yes, that's a non-issue.

edit: only if nothing has been migrated to 20200223 by 3.2.45.

poire-z · 2020-08-30T12:54:35Z

(I really didn't want to mention this :/ but so you know :)
With this legacy rendering changes picked in KOReader, I get FB2 footnotes no more run-in/stuck to the text:

Previously, these were "erm_final inside erm_final", but I killed that possibility. Dunno if there's some quick and simple solution.
I hope you won't fight much more with resurrecting legacy mode, when one can just switch to enhanced flat mode :)

virxkane · 2020-08-30T17:36:22Z

With this legacy rendering changes picked in KOReader, I get FB2 footnotes no more run-in/stuck to the text:

Yes, that should be fixed.

I hope you won't fight much more with resurrecting legacy mode, when one can just switch to enhanced flat mode :)

Why then did you introduce it, legacy rendering, which is actually a kind of simplified and not truly legacy?
And if it is not needed, then maybe delete it?

poire-z · 2020-08-30T18:28:26Z

I did not introduce it ! I just dared not killing it :) and it costed nothing keeping it (except these painful moments these days :)
Rename it not-truly legacy if you don't want to feel obliged being exactly like previously (you're fixing edges cases... and introducing others by doing so, maybe endlessly), or just don't propose it in the UI.
But sometimes, it's a one-stop solution to have plain flat no bells and whistles rendering which might be good enough with complicated books - and is usually fine enough in the non-edge cases.

Not sure we can just delete it. We need to support rend_flags=0 (no float/inlinebox/incompletetable) for old books and migrating their xpointers. After that, it's indeed only rendering and may be it would work rendering rend_flags=0 with renderBlockElementEnhanced(), but I don't know and don't want to have to know how/if it will render correctly such DOM.
And it's a good debugging tool to know in which part of the code stuff might be broken.

virxkane · 2020-08-30T21:19:09Z

when one can just switch to enhanced flat mode

Try to tell everyone of "playstore and tons of anonymous users".

I did not introduce it ! I just dared not killing it :)

Ok, you introduce term "legacy" render mode and macro "BLOCK_RENDERING_FLAGS_LEGACY" :)

Rename it not-truly legacy if you don't want to feel obliged being exactly like previously (you're fixing edges cases... and introducing others by doing so, maybe endlessly), or just don't propose it in the UI.

But really, what will suffer if "legacy" rendering mode will be removed (only in UI?) and use "flat" instead? Bookmarks shouldn't be affected, right?

Not sure we can just delete it. We need to support rend_flags=0 (no float/inlinebox/incompletetable) for old books

By "inlinebox" you mean "p" inside "span"?

And then I wrote another crutch based on yours :)

@@ -2527,6 +2536,21 @@ void renderFinalBlock( ldomNode * enode, LFormattedText * txform, RenderRectAcce
         if (parent && parent->isNull())
             parent = NULL;
 
+        if (!BLOCK_RENDERING_G(ENHANCED)) {
+            if (is_block && (flags & LTEXT_FLAG_NEWLINE) ) {
+                // Nodes with "display: block" but in one section with node "display: run-in"
+                // must be rendered as inline nodes in legacy rendering mode.
+                if ( enode->getNodeIndex() > 0 && parent && parent->getChildCount() > 1 ) {
+                    ldomNode * first_sibling = parent->getChildNode(0);
+                    if (first_sibling && !first_sibling->isNull() && first_sibling->isElement()) {
+                        if (first_sibling->getStyle()->display == css_d_run_in) {
+                            is_block = false;
+                            flags &= ~LTEXT_FLAG_NEWLINE;
+                        }
+                    }
+                }
+            }
+        }
         // Nodes with "display: run-in" are inline nodes brought at start of the final node
         bool isRunIn = style->display == css_d_run_in;
         if ( isRunIn ) {

poire-z · 2020-08-30T21:40:53Z

Ok, you introduce term "legacy" render mode

Yes, that will teach me to be polite... I should have called a cat a cat and called it "buggy hacky old" render mode :)

But really, what will suffer if "legacy" rendering mode will be removed (only in UI?) and use "flat" instead? Bookmarks shouldn't be affected, right?

I guess it should be fine.
But for old documents, you still need to build the DOM once with rend_flags=0 (even if you don't render it), migrate xpointers, and then re-build the DOM with rend_flags for flat and renderEnhanced.

By "inlinebox" you mean "p" inside "span"?

No, P inside SPAN is just a particular case of that. inlineBox wraps anything with display:inline-block or inline-table (which usually don't take the full width, it's like an inline image, except that it's a HTML snippet).
P inside SPAN is just me finding out I could solve this issue of block among inline by rendering as if (with the same code) it was . Just a trick. Before that, they were resetRecursiveToInline().

And then I wrote another crutch based on yours :)

Should work :)
May be, to make it explicite you're not processing the main erm_final node (because its parent and siblings have nothing to do in the matter :):

-if (is_block && (flags & LTEXT_FLAG_NEWLINE) ) {
+if (rm != erm_final && is_block && (flags & LTEXT_FLAG_NEWLINE) ) {

Also, I think you want to do it only for the first node following a first_sibling with css_d_run_in. The next ones shouldn't be affected

<div>
  <p style="display: run-in">123</p>
  <p>First line</p> <!-- only this one should follow 123 -->
  <p>Second line</p>
</div>

virxkane · 2020-08-30T22:16:07Z

Should work :)

It works.

May be, to make it explicite you're not processing the main erm_final node (because its parent and siblings have nothing to do in the matter :)

A sensible remark, but I found that first line after number (in title) in footnotes section have rendering method=erm_inline, but second - erm_final and than with this check

Also, I think you want to do it only for the first node following a first_sibling with css_d_run_in. The next ones shouldn't be affected
<div>
 123
 First line 
 Second line
</div>

already not affected.
Related to ldomNode::initNodeRendMethod() ?
But of course it is not diffucult replace enode->getNodeIndex() > 0 with enode->getNodeIndex() == 1.

poire-z · 2020-08-30T22:59:11Z

Related to ldomNode::initNodeRendMethod() ?

Looks like when run-in, we made it so we only have the run-in and the next block (reset to erm_inline), only these 2 , in the erm_final we're rendering (any other P sibling is in its own erm_final that we're not processing here.
Your solution in #160 looks fine.

poire-z · 2020-08-31T06:37:12Z

Picked #160, and I got the footnote text centered:

Not on your side?

(Feels like the next isRunIn check that is supposed to pick the text-align from the next sibling has not done what is expected - but we go thru it.)

virxkane · 2020-08-31T06:50:49Z

@poire-z At least it doesn't look like that with PR # 160.

poire-z · 2020-08-31T07:03:22Z

And before #160, you had the footnote numbers centered, like on my screenshots at #157 (comment) (just to be sure it's not differences in our fb2.css) ?

virxkane · 2020-08-31T07:09:12Z

And before #160, you had the footnote numbers centered, like on my screenshots at #157 (comment) (just to be sure it's not differences in our fb2.css) ?

No, not centered.

poire-z · 2020-08-31T07:15:33Z

OK.
You have:

title p, h1 p, h2 p { 
    $title.all
}

where we have:

title p, subtitle p, h1 p, h2 p, h3 p, h4 p, h5 p, h6 p {
        text-align: center;
        text-indent: 0px
}

So, I guess you have that configurable in the UI. And may be if you can set titles centered there, you'll get a rendering like I do.

virxkane · 2020-08-31T07:20:49Z

So, I guess you have that configurable in the UI. And may be if you can set titles centered there, you'll get a rendering like I do.

I hope you won't fight much more with resurrecting legacy mode, when one can just switch to enhanced flat mode :)

Can I stop there? :)

poire-z · 2020-08-31T08:50:32Z

I guess it's because there is something caught by your is_block check that is inside the run-in:

 <body name="notes">
  <section id="fn1">
   <title>    <!-- run-in , ok, we pick textalign from next sibling block2 -->
    <p>1</p>  <!-- block1 , centered, but as is_block=true, this resets what we pick from block2 -->
   </title>
   <p>some text</p> <!-- block2, left , is_block=false cause sibling of run-in -->
  </section>

I guess an additional hack to reset is_block to false when a parent is run_in might be needed :/

poire-z · 2020-08-31T13:28:31Z

This seems to be enough:

         bool is_block = rm == erm_final;
         if (!BLOCK_RENDERING_G(ENHANCED) && !is_block) {
             // In legacy rendering mode, we should get the same text formatting flags
             // as in CoolReader 3.2.38 and earlier, i.e. set is_block to true for
             // any block elements.
             is_block = style->display >= css_d_block;
             if ( rm != erm_final && is_block ) {
                 // Hack for "legacy" rendering mode:
                 // First node with "display: block" after node "display: run-in" in one section
                 // must be rendered as inline node.
-                if ( enode->getNodeIndex() == 1 && parent && parent->getChildCount() > 1 ) {
+                if ( enode->getNodeIndex() == 1 ) {
                     ldomNode * first_sibling = parent->getChildNode(0);
                     if (first_sibling && !first_sibling->isNull() && first_sibling->isElement()) {
                         css_style_ref_t fs_style = first_sibling->getStyle();
                         if (!fs_style.isNull() && fs_style->display == css_d_run_in) {
                             is_block = false;
                         }
                     }
                 }
+                if (is_block) {
+                    // Also checks this block is not contained in a run-in,
+                    // in which case we should keep it inline
+                    ldomNode * n = enode;
+                    while ( n && n->getRendMethod() != erm_final ) {
+                        if ( n->getStyle()->display == css_d_run_in ) {
+                            is_block = false;
+                            break;
+                        }
+                        n = n->getParentNode();
+                    }
+                }
             }
         }
         lUInt32 flags = styleToTextFmtFlags( is_block, style, baseflags, direction );

Given that we go in this block only for edge cases of blocks among inline, the additional cost of checking parents shouldn't be triggerred often (except for FB2 footnotes, but well :).

(Shortened your first check, as enode->getNodeIndex() == 1 ensures the rest is true.)

virxkane · 2020-08-31T16:44:58Z

@poire-z Sorry, I'm done.

virxkane and others added 19 commits August 29, 2020 16:53

Fixed fb2 coverpage drawing in scroll mode (continuous) with enhanced…

2fea5d4

… render. With corrective patch @poire-z.

fb2.css: use OTF tabular-nums for footnote numbers

5e305e0

If the font provides the 'tabular-nums' (tnum) OpenType feature, we'll get more properly aligned starts of text after the numbers.

Text: ignore ascii and unicode control chars

7f13ce3

Some of these can still be found in old documents or badly converted ones, and most fonts have no glyph for them (and if they do, it's a small symbol with no meaning for the reader).

Fix HR positionning when floats involved

7100c63

Strangely, when floats are involved, a HR behaves differently than a regular DIV and adjust to the available space. Nothing mentionned about that in the specs, so try to handle them as Firefox does.

writeNodeEx(): minor tweaks

42ed15c

Avoid crash when called while styles not yet set or reset. Show the rootnode as <?RootNode?>.

OnTagClose(): add self_closing_tag parameter

92af2c0

So DOM writers can distinguish from and handle these differently.

HTML format detection: accept HTML5 doctype

ffeda1d

Accept <!DOCTYPE html> which may not have any <html>, <head> and <body> tags (they are then implicit), and any other kind of <!DOCTYPE ... HTML ...> which are obviously HTML, no matter how broken they are.

HTML parser: rework Lib.ru specific handling

8475b5b

Avoid the first (wrong) rendering from being different than next (good) ones, because it was parsed as PRE and the node was substituted to a DIV when closed. Handle Lib.ru books in a more correct DOM building way.

HTML parser: ignore XML processing instructions (#371)

ee9584a

<?xml?> and the like are parsed and fed to the DOM writers, but let's ignore them and not include them in the DOM tree.

Android: use fast (not precise) mode for functions LVDocView::getBook…

8755c3a

…mark() **only** for page turn & text scroll. Also don't call LVDocView::getPageDocumentRange() in fast mode.

Optimized calculation of average animation duration.

43ae549

Android: fixed scrolling of text around start and end of a book in sc…

32d7370

…rolling mode.

Improved methods for selecting next/previous sentence when using "***…

e7385e2

…" paragraph separators. In fact, this is a continuation of PR #144, i.e. restoration of old functionality.

Legacy rendering mode: display block elements that are inside inline …

afc16d0

…elements as block elements.

Trying to update the structure of a database that has been modified b…

f1d4861

…y some kind of inconsistent fork of the program.

Forced update the DOM level in database from previous newest to lates…

c828634

…t newest. Allowed to changing the DOM level and block rendering flags in options dialog for CHM files.

buggins merged commit 8dc8698 into buggins:master Aug 30, 2020

virxkane mentioned this pull request Aug 30, 2020

Fixed footnotes rendering in legacy rendering mode that is broken in … #160

Merged

virxkane deleted the updates-202008-1 branch September 1, 2020 16:38

poire-z mentioned this pull request Oct 1, 2020

(Upstream) replace global variables, some legacy text rendering fixes koreader/crengine#384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates 2020.08-1 #157

Updates 2020.08-1 #157

virxkane commented Aug 29, 2020

poire-z commented Aug 29, 2020

virxkane commented Aug 29, 2020 •

edited

poire-z commented Aug 29, 2020 •

edited

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020 •

edited

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020

poire-z commented Aug 31, 2020

poire-z commented Aug 31, 2020 •

edited

virxkane commented Aug 31, 2020

Updates 2020.08-1 #157

Updates 2020.08-1 #157

Conversation

virxkane commented Aug 29, 2020

poire-z commented Aug 29, 2020

virxkane commented Aug 29, 2020 • edited

poire-z commented Aug 29, 2020 • edited

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

virxkane commented Aug 30, 2020

poire-z commented Aug 30, 2020

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020 • edited

poire-z commented Aug 31, 2020

virxkane commented Aug 31, 2020

poire-z commented Aug 31, 2020

poire-z commented Aug 31, 2020 • edited

virxkane commented Aug 31, 2020

virxkane commented Aug 29, 2020 •

edited

poire-z commented Aug 29, 2020 •

edited

virxkane commented Aug 31, 2020 •

edited

poire-z commented Aug 31, 2020 •

edited