bump crengine, migrate books to normalized xpointers by poire-z · Pull Request #5897 · koreader/koreader

poire-z · 2020-02-24T15:08:48Z

Will need a base bump for koreader/koreader-base#1054:
Includes koreader/crengine#328 :

Fix shifted native selection/internal bookmarks in legacy mode
Fix drawing of inline-block taller than page height
(Upstream) createXPointer/toString: normalized xpointers
toStringUsingNames(): rename to toStringV2()
DOM_VERSION_WITH_NORMALIZED_XPOINTERS 20200223
CSS/List items: skip boxing elements when walking
CSS: parse pseudoclass nth_child(3n+2)
toStringV2(): output [1] when first but not single
Tables: complete incomplete tables
Store gDOMVersionRequested in cache file header
Cache: increase Rects cache size

cre.cpp: adds getDomVersionWithNormalizedXPointers() and getNormalizedXPointer(xpv1)

When opening a book previously opened with an older DOM version, a migration/upgrade to use the latest one, and migrate previous unnormalized XPointers to normalized ones is proposed (previous metadata.lua is backup'ed as metadata.epub.lua.old_dom20180528).
Currently with those kind of ConfirmBoxes:

(I'm not a big highlighter, and I think I've seen that 3rd one only on test html files that I often edited to add test cases, so easily inducing shifts in the DOM and breaking past highlights.)

I don't know if we should really show these messages, or just do the migration in the 1) and 2) cases, which could just look like 2 consecutive openings of the same document.
I guess we should show the 3rd, even if I'm not sure the user will be able to get these lost ones back.
May be let them for a few weeks in the nightlies, so testers can see and tell, and we'll then decide if it's worthwhile showing them? Should I still let these message translatable then?

The style tweaks about list items (added to the Paragraphs> section ) might help getting back bullets when the publisher uses features that we don't support, like:

ul.tiret, ul.lettremin, ul.lettremaj {
    list-style-type: none;
}
ul.tiret > li:before {
    content: '-\00a0';
    float: left;
}

This change is

poire-z · 2020-02-24T15:12:20Z

Pinging @noembryo for info: there's a new cre_dom_version upgrade, so new books will get it. I dunno how the proposed cre_dom_version upgrade, and so a change of it for a same book, will affect KOHighlights.
Pinging @mustafa-001 for info too: shouldn't matter much for your implemenation/migration to annotations, except the added little bit of code that loop thru bookmarks and highlight will have to be extended to loop also thru your future annotations.

Just like what's been done previously for InfoMessage. Also tweak 'other_buttons' option (not yet used anywhere) to allow multiple rows of such other buttons.

noembryo · 2020-02-24T15:19:41Z

@poire-z Yeap.. KOHighlights checks the cre_dom_version when determining if two books (the same book in two different readers) can be synced.
If they are different, there is no option for sync.

Frenzie · 2020-02-24T15:19:52Z

+    end
+
+    local text = _([[
+This book has been first opened, and handled since, by some old version of the rendering code.


Perhaps it can be slightly more succinct?

This book was first opened by an older version of the rendering code.

Bookmarks and highlights can be upgraded to the latest version of the code.

%1

Proceed with this upgrade and reload the book?

I find that a bit too succint :) but well, as the author of the verbose variant, I can't say much :)
Any other opinion ?
(out for a bit, will rethink that later)

I kinda like it, I'd possibly switch the second sentence to "You won't lose your bookmarks & highlights" or something similar.

The end goal being hiding the technicalities, and just basically saying "we need to reload the book, here's what it'll mean for you in practical terms".

ok ok :)
I'd like to keep the , and handled since, by... - because, with just was first opened by, there's some continuity/logic missing. ("OK, I opened it previously, but why this upgrade now?")
Fine with keeping it?

And so:
Bookmarks and highlights can be upgraded to the latest version of the code.
should replace the 2nd sencence of the first paragraph, or also the 2nd paragraph when all is good (All your bookmarks and highlights are valid and will be available after the migration) ?

What about the 2 possible (but rare in practice I think) "Note that..." ?

This book is currently handled by an older version of the rendering code?

That's quite ok, but it doesn't tell why it is currently handled by... to the curious user :)

Frenzie · 2020-02-24T15:21:10Z

+        other_buttons = {{
+            {
+                -- this is the real cancel/do nothing
+                text = _("Ask me next time"),


Bit long?

Ask again

Not now

The awful Apple way to deal with that in notifications is Snooze.

I kinda like Not now, FWIW ;).

poire-z · 2020-02-24T15:25:30Z

I hope that this is updated with every metadata write.

it's updated once, when the user hits "Upgrade now", then it's stored as cre_dom_version=20200223 and there's no reason for it to go back to the old one, on the next metadata saves.

Just wondering if you should or not make an exception for your cre_dom_version coherency check, and allow synchronizing current book with 20200223 to your cached data for that same book when it had 20180528 or older: the pos0/pos1 may be the same or may have changed, but the text and notes will be the same.

poire-z · 2020-02-24T16:52:54Z

Enable new rendering feature COMPLETE_INCOMPLETE_TABLES on all enhanced rendering mode, but have it disabled for earlier cre_dom_version. Also increase default_cre_storage_size_factor from 20 to 40.

noembryo · 2020-02-24T18:46:11Z

@poire-z The problem is that the wrong pos0/pos1 could end up in a reader display (1st -current- reader>sync with db, then db>sync with 2nd -older version- reader) and that would not look good.. :o(

poire-z · 2020-02-24T22:36:02Z

OK, merging.
Feel free to reword it after you've seen it on your device.
And if you got tired of it, we could add an option "Upgrade always and don't ask me", to be done only when all bookmarks and highlights are found.
Also, just realizing that I didn't do anything special when there are no bookmark and no highlight...
So, feedback still welcome on how to do/skip that better.

poire-z · 2020-03-08T14:07:52Z

Something a bit unfortunate that I just realized now:

koreader/frontend/apps/reader/modules/readerrolling.lua

Lines 1277 to 1289 in 1d58eb8

    
           UIManager:show(ConfirmBox:new{ 
        
               text = text, 
        
               other_buttons = {{ 
        
                   { 
        
                       -- this is the real cancel/do nothing 
        
                       text = _("Not now"), 
        
                   } 
        
               }}, 
        
               cancel_text = _("Not for this book"), 
        
               cancel_callback = function() 
        
                   self.ui.doc_settings:saveSetting("cre_keep_old_dom_version", true) 
        
               end, 
        
               ok_text = _("Upgrade now"),

Dismissing that ConfirmBox by taping outside of it (which is quite common for me) will then have "Not for this book" setting saved - and so one will never see that ConfirmBox anymore for this book (until one goes removing that setting from metadata.epub.lua)
Might be considered a design choice from the Dont-Bother-Users-Front - but I would have prefered "Not now"... :|

Frenzie · 2020-03-08T16:05:27Z

I agree, not now is probably better for that.

poire-z · 2020-03-08T16:13:59Z

Given the button geometry/length (cancel|OK) always on the first row, other rows added after, and cancel being the one triggered when taping outside, I guess the easier solution is just to use dismissable = false on that ConfirmBox.
And given that you just made a release, and this is probably not worth making another one, may be I should then use a different setting than cre_keep_old_dom_version so people who dismissed it in 2020.03 get that ConfirmBox again in 2020.04 ?

Frenzie · 2020-03-08T16:19:15Z

I suppose so, although getting the dialog again might be slightly annoying too.

noembryo · 2020-03-08T16:31:52Z

Re-releasing the release (with a .1 bump) is out of the question?

Frenzie · 2020-03-08T17:40:11Z

Fine by me, but it'd better to wait a couple of days for that in case something actually important pops up.

yparitcher · 2020-03-13T04:39:45Z

Is there a way i can make my books auto upgrade? i rarely use bookmarks, and do not want to have to confirm for every book.
Thanks

Edit:

I don't know if we should really show these messages, or just do the migration in the 1) and 2) cases, which could just look like 2 consecutive openings of the same document.

like @poire-z sugested above #5897 (comment) auto migrate in case 1) & 2)

poire-z · 2020-03-13T20:24:22Z

Is there a way i can make my books auto upgrade?

Manually :) you could grep/sed all your .sdr/metadata.epub.lua to set ["cre_dom_version"] = 20200223,

auto migrate in case 1) & 2)

It might be a bit late 18 days after this went it, and now that we made some releases, to change this - users might get used and expect to see this to be confortable knowing their book were upgraded.

So, I guess it could/should be some user manual choice to have that migration done from now on without asking. How, not sure:

new buttons at the bottom of this ConfirmBox "Upgrade all books"
followup confirmbox when one has tapped "Upgrade now", to ask him if we should do that without asking next time (asking that once or always?)
hold on "Upgrade now" to make it the default (but no support yet for hold on ConfirmBox buttons).

Style tweaks: force List items bullet/decimal style

c81afba

ConfirmBox: avoid overflowing screen height when long text

75464c8

Just like what's been done previously for InfoMessage. Also tweak 'other_buttons' option (not yet used anywhere) to allow multiple rows of such other buttons.

poire-z force-pushed the dom_version_20200223 branch from e3a5b49 to b07c214 Compare February 24, 2020 15:17

Frenzie reviewed Feb 24, 2020

View reviewed changes

Frenzie added this to the 2020.03 milestone Feb 24, 2020

Frenzie added CREngine enhancement labels Feb 24, 2020

bump crengine, migrate books to normalized xpointers

1433eb0

Enable new rendering feature COMPLETE_INCOMPLETE_TABLES on all enhanced rendering mode, but have it disabled for earlier cre_dom_version. Also increase default_cre_storage_size_factor from 20 to 40.

poire-z force-pushed the dom_version_20200223 branch from b07c214 to 1433eb0 Compare February 24, 2020 16:56

poire-z merged commit 5a4f5b4 into koreader:master Feb 24, 2020

poire-z deleted the dom_version_20200223 branch February 24, 2020 22:36

poire-z mentioned this pull request Mar 8, 2020

Make xpointers migration ConfirmBox non-dismissable #5931

Merged

poire-z mentioned this pull request Apr 6, 2022

koreader doesn't ask for upgrade when cre_dom_version is newer #8973

Closed

coderabbitai Bot mentioned this pull request Apr 17, 2026

fix: Switch to xpath map for paragraph level syncing in KOSync crosspoint-reader/crosspoint-reader#1686

Merged

Uh oh!

Conversation

poire-z commented Feb 24, 2020 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noembryo commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiLuJe Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z commented Feb 24, 2020

Uh oh!

poire-z commented Feb 24, 2020

Uh oh!

noembryo commented Feb 24, 2020

Uh oh!

poire-z commented Feb 24, 2020

Uh oh!

poire-z commented Mar 8, 2020

Uh oh!

Frenzie commented Mar 8, 2020

Uh oh!

poire-z commented Mar 8, 2020

Uh oh!

Frenzie commented Mar 8, 2020

Uh oh!

noembryo commented Mar 8, 2020

Uh oh!

Frenzie commented Mar 8, 2020

Uh oh!

yparitcher commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

poire-z commented Feb 24, 2020 •

edited by Frenzie

Loading

poire-z commented Feb 24, 2020 •

edited

Loading

noembryo commented Feb 24, 2020 •

edited

Loading

NiLuJe Feb 24, 2020 •

edited

Loading

yparitcher commented Mar 13, 2020 •

edited

Loading