Skip to content
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, migrate books to normalized xpointers #5897

Merged
merged 3 commits into from Feb 24, 2020

Conversation

@poire-z
Copy link
Contributor

poire-z commented Feb 24, 2020

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:

image

image

image
(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 Reviewable

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 24, 2020

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.
@poire-z poire-z force-pushed the poire-z:dom_version_20200223 branch from e3a5b49 to b07c214 Feb 24, 2020
@noembryo

This comment has been minimized.

Copy link
Contributor

noembryo commented Feb 24, 2020

@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.

end

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Feb 24, 2020

Member

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?

This comment has been minimized.

Copy link
@poire-z

poire-z Feb 24, 2020

Author Contributor

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)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Feb 24, 2020

Member

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".

This comment has been minimized.

Copy link
@poire-z

poire-z Feb 24, 2020

Author Contributor

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 comment has been minimized.

Copy link
@Frenzie

Frenzie Feb 24, 2020

Member

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

This comment has been minimized.

Copy link
@poire-z

poire-z Feb 24, 2020

Author Contributor

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

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

This comment has been minimized.

Copy link
@Frenzie

Frenzie Feb 24, 2020

Member

Bit long?

  • Ask again
  • Not now

This comment has been minimized.

Copy link
@Frenzie

Frenzie Feb 24, 2020

Member

Not yet

This comment has been minimized.

Copy link
@poire-z

poire-z Feb 24, 2020

Author Contributor

Ask later?

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Feb 24, 2020

Member

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

I kinda like Not now, FWIW ;).

@Frenzie Frenzie added this to the 2020.03 milestone Feb 24, 2020
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 24, 2020

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

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 24, 2020

image

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 poire-z force-pushed the poire-z:dom_version_20200223 branch from b07c214 to 1433eb0 Feb 24, 2020
@noembryo

This comment has been minimized.

Copy link
Contributor

noembryo commented Feb 24, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 24, 2020

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 poire-z merged commit 5a4f5b4 into koreader:master Feb 24, 2020
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:dom_version_20200223 branch Feb 24, 2020
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2020

Something a bit unfortunate that I just realized now:

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

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2020

I agree, not now is probably better for that.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2020

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

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2020

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

@noembryo

This comment has been minimized.

Copy link
Contributor

noembryo commented Mar 8, 2020

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

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2020

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

@yparitcher

This comment has been minimized.

Copy link
Contributor

yparitcher commented Mar 13, 2020

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

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 13, 2020

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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.