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

bump crengine: HTML and rendering fixes #3992

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
3 participants
@poire-z
Contributor

poire-z commented Jun 1, 2018

Includes:

Also update Wikipedia epub.css stylesheets regarding recent HTML fixes and enhancements.
Remove "Remove all borders" workaround tweak, as it's now fixed.

Some class name based styles have been removed from epub.css, mostly some stuff related to the FB2 format that was probably the first format supported by Cool Reader. For those nostalgic of these added styles, here's a file with them that can be put in koreader/styletweaks and applied via User style tweaks:
CoolReader epub additions.css.txt

bump crengine: HTML and rendering fixes
- Fix getting xpointer to current page
- epub.css cleanup
- fb2def.h update
- Fix a few edge-case rendering issues
- Fix border being drawn in spite of 'border-width: 0'

Also update Wikipedia epub.css stylesheets regarding
recent HTML fixes and enhancements.
Remove "Remove all borders" workaround tweaks, as it's
now fixed.
@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 1, 2018

OK, there is a thing with my change at:
https://github.com/koreader/crengine/pull/197/files#diff-e55744b4bf59e4459d1afb4ab2dd6022L1350
On juliet.epub, this cause a blank page at book start, the cover being on page2 (but when opening the book with a purge sidecar, we are put on page 2... not on that unit test that requests page 1 explicitely thus)

spec/front/unit/readerfooter_spec.lua:154: Expected objects to be the same.
Passed in:
(string) '1 / 205 | 21:24 | => 2 | B:0% | R:0% | TB: na | TC: na'
Expected:
(string) '1 / 205 | 21:24 | => 1 | B:0% | R:0% | TB: na | TC: na'

Probably a LTEXT_FLAG_NEWLINE at start of book that makes the 100% height cover go to page 2...
@NiLuJe @Frenzie : a bit late for me to work out that thing, and given that @NiLuJe #3983 and this PR have base bumps interleaving, I can't bump crengine with a fix without bumping #3983 base.

So, I'd say we merge this PR in spite of the unit test failure, @NiLuJe merges his, and I'll cancel the nightly pipeline tomorrow morning and hopefully i'll find some fix to crengine.
(Or we don't even cancel it, the nightly with this small bug won't mess any DOM or past highlights as it's just a post-DOM rendering issue)

(The good news for me was that fix unit tests by requesting older dom version actually made 2 unit tests works again, so it works as intended :) I'll see about updating these units tests to the current DOM version later).

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 1, 2018

I'm off to bed. @NiLuJe , if you're not :) , and you agree with the above, feel free to merge this one if you want to go on with yours. Or else, we'll work that out tomorrow.

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented Jun 1, 2018

I'm in absolutely no hurry to get #3983 in, so, everyone take your time, I'll re-bump base in #3983 once everything's okay. In the end, I'll squash it anyway (and it's in a branch on my end, so the history can stay fully available, for science!), so, not a problem, we can all stare at a little red cross for a while longer ;).

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 1, 2018

But if I bump a new crengine in base and want to bump it in this PR, this will carry your merged Proper REAGL handling on Kobo Aura (#676) with it.
https://github.com/koreader/koreader-base/commits/master
Will that work bumped into koreader if your #3983 is not (no brekage to the existing framebuffer API)?

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented Jun 1, 2018

It shouldn't cause any issue, it only adds and fixes stuff. The trickiest thing might be the BGR stuff, but the missing bits should evaluate to nil and default to RGB as before.

The (base) CI passed cleanly without the KO bits, so that shouldn't be a worry either ;).

luacheck (in KO) might complain about some stuff (I'm thinking of the missing hasBGRFrameBuffer in Device), but that's the extent of it ;).

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jun 2, 2018

Agreed, no need to hurry. In fact if we wait we could probably call the next release stable?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 2, 2018

Mhhh, about this blank first page on juliet.epub...
Quite tough to understand why a change happened, but here's what I got.
Built DOM from the first 2 fragments:

<?xml version="1.0"?>
<body>
  <DocFragment id="_doc_fragment_0">
    <body>
      <div style="text-align: center">
        <img src="OPS/images/cover.png" alt="Cover" style="display:inline"/>
      </div>
    </body>
  </DocFragment>
  <DocFragment StyleSheet="OPS/css/title.css" id="_doc_fragment_1">
    <body>
      <div class="body">
        <div class="logo"><img src="OPS/images/logo-feedbooks-tiny.png" alt="Feedbooks" title="Feedbooks"/>some text that I added to check</div>
        <h1>Romeo and Juliet</h1>
        <h2><span/>William Shakespeare</h2>
      </div>
      <div class="infos"><br/><b>Published:</b> 1597<br/> <b>Categorie(s):</b> Fiction, Drama, Romance<br/> <b>Source:</b> http://shakespeare.mit.edu</div>
    </body>
  </DocFragment>
</body>

Before my change mentionned above, we got:
Page 1: cover image
Page 2: blank page
Page 3: Feedbooks NEWLINE some text that I added to check
Page 4: Romeo and Juliet

Some logs:

60 1 1 1 1   img inline
  no clear newline flag
16 2 0 1 1    div block
  clear newline flag
Resize image (520x800) max 580x578 block
2Resize image (520x800) max 580x578 arbitrary  *1 => w=375 h=577
Resize image (520x800) max 580x578 block
2Resize image (520x800) max 580x578 arbitrary  *1 => w=375 h=577

After my change:
Page 1: blank page
Page 2 cover image
Page 3: blank page
Page 4: Feedbooks NO_NEWLINE some text that I added to check
Page 5: Romeo and Juliet

Some logs:

60 1 1 1 1    img inline
  clear newline flag 1 > 0
16 2 0 1 1    div block
  clear newline flag 2 > 0
Resize image (520x800) max 580x578 inline
2Resize image (520x800) max 580x578 integer  *1 => w=375 h=578
Resize image (520x800) max 576x578 inline
2Resize image (520x800) max 576x578 integer  *1 => w=375 h=578

I get the exact same flags stuff for the cover image and the Feedbooks image, so I guess my change is valid to solve the NEWLINE if image first child followed by some text.
Which is also what happens with the first image: the spaces in the HTML make for some text after the cover image. If I remove all the spaces to get:
<body><div style="text-align: center"><img src="images/cover.png" alt="Cover" style="display:inline"/></div></body>
I get no blank page at start.

Anyway, the thing is that (not even sure, as all this code is quite complicated and I don't yet grasp all of it)

  • before my change: image and following text on 2 lines => image is alone and some code in lvtextfm.cpp check for the nb of source fragments to decide (that seems wrong) if it is inline or block, and which image zooming option to use => it choose block/arbitraty (as the image is alone), which made an image of size 577.
  • after my change: image and following text on same line => image is not alone and some code in lvtextfm.cpp check for the nb of source fragments to decide (that seems wrong) if it is inline or block, and which image zooming option to use => it choose inline/integer (as it must be inline, if it is not alone), which made an image of size 578.

Then, I guess, the lvpagesplitter code has some block of 578+1px+1px because of this in our epub.css:

div {
    margin: 1px;
}

580 does not fit on a page, and some code I added months ago decides to push that on the next page.
When the image is 577px + 1px + 1px, 578 or 579 fits on first page, so it let it be there.

I guess the lvpagesplitter code needs some tweak: if that block has a height larger than screen height and there's nothing before it: no need to push that on next page where it will be spliced anyway: just do that on current page.

BUT, a quick solution which fixes this problem is just to remove the div { margin: 1px; } from epub.css (I hesitated doing that last week, as it seems unnecessary and may cause small alignments problems when multiple imbricated DIV).
Without that added 1px (or 2) to the DIV containing the image, the block fits on the first page, and there's not more blank pages

With my change and no div { margin: 1px; }, we would get:
Page 1 cover image
Page 2: Feedbooks NO_NEWLINE some text that I added to check
Page 3: Romeo and Juliet

So, should I just do that: remove div { margin: 1px; } and solve another blank page bug?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 2, 2018

@frankyifei : do you remember why you added margin: 1px; to DIV in koreader/crengine#40 ? (debugging aid that you forgot to remove, or did it fix something?)

@poire-z poire-z force-pushed the poire-z:bump_crengine branch from a622ecb to e5b5bbf Jun 2, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jun 2, 2018

OK, I get many unit test failures when bringing @NiLuJe base with me:

ffi/framebuffer.lua:363: attempt to call method 'hasBGRFrameBuffer' (a nil value)

stack traceback:
	ffi/framebuffer.lua:363: in function 'shot'
	spec/front/unit/readerhighlight_spec.lua:89: in function <spec/front/unit/readerhighlight_spec.lua:87>

I guess for that, and for coherency/cleanness of the koreader+base bumps, it's cleaner to:

  1. bump this PR with only the original commit, with 3 unit test failures
  2. bump @NiLuJe #3983 with its base => still my 3 unit test failures
  3. I go on with bumping crengine and fixing these unit test failures

@poire-z poire-z force-pushed the poire-z:bump_crengine branch from e5b5bbf to 32a7d2b Jun 2, 2018

@poire-z poire-z merged commit bdbaa7d into koreader:master Jun 2, 2018

1 check failed

ci/circleci Your tests failed on CircleCI
Details

@poire-z poire-z deleted the poire-z:bump_crengine branch Jun 2, 2018

@NiLuJe

This comment has been minimized.

Member

NiLuJe commented Jun 2, 2018

Yeah, that's the one I was worried about, sorry about that -_-".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment