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

[feat] Add MuPDF EPUB/FB2 dynamic font size #5282

Merged
merged 5 commits into from Aug 30, 2019

Conversation

@Frenzie
Copy link
Member

commented Aug 29, 2019

Closes #4368.

I might leave it like this for now (after some minor cleanup), meaning the full document will have to be reloaded, because layoutDocument() has to be called again. Ideally I'd give it that bit of polish too. Rerendered on the fly.

@Frenzie Frenzie added the enhancement label Aug 29, 2019

@Frenzie Frenzie added this to the 2019.09 milestone Aug 29, 2019

@@ -954,7 +954,8 @@ function ReaderPaging:_gotoPage(number, orig_mode)
end
if number > self.number_of_pages or number < 1 then
logger.warn("wrong page number: "..number.."!")
return false
number = self.number_of_pages

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 29, 2019

Author Member

Font size changes cause invalid page numbers. This function only protects against invalid links or something like that, not against invalid last_page settings. I think that's probably a more general issue, because what if you cut off a few pages from a PDF or something?

Perhaps I should add a unit test against this crash.

frontend/document/pdfdocument.lua Outdated Show resolved Hide resolved

@Frenzie Frenzie force-pushed the Frenzie:mupdf-epub-fontsize branch from 2aa94b6 to 97300fa Aug 30, 2019

@Frenzie Frenzie changed the title WIP [feat] Add MuPDF EPUB/FB2 dynamic font size [feat] Add MuPDF EPUB/FB2 dynamic font size Aug 30, 2019

@@ -36,12 +33,11 @@ function PdfDocument:init()
if not ok then
error(self._document) -- will contain error message
end
self.epub_font_size = self:getKoptFontSize()

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 30, 2019

Contributor

Looks ok as a whole.
Except for the namings which make it a bit hard to follow :)
self:getKoptFontSize() sounds generic, but it returns something that matters only for reflowable documents.
epub_font_size sounds obvious, but might be better to name it reflowable_font_size and stay with "reflowable" in the related functions and variables names, like the nicely named self.is_reflowable and document:isDocumentReflowable()

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 30, 2019

Author Member

Should be better now.

self.is_open = true
self.is_reflowable = self._document:isDocumentReflowable()

This comment has been minimized.

Copy link
@poire-z

poire-z Aug 30, 2019

Contributor

nitpick: might move this one up so all the "reflowable" properties are close together

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 30, 2019

Author Member

True, it's available right after first opening the document.

Btw, not implemented here but fz_make_bookmark and fz_lookup_bookmark should be used before & after redoing the layout to return to the same location in the reflowable document.

@Frenzie Frenzie merged commit f1f6eeb into koreader:master Aug 30, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:mupdf-epub-fontsize branch Aug 30, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

In case you feel like fixing/enhancing on that reflowable mupdf front, it currently crashes when following footnotes link (just open any wikipedia epub), because of link_url being nil at that point:

local is_http_link = link_url:find("^https?://") ~= nil

Before reflowable, that code was only expecting a page attribute (internal links pointed to page numbers with PDF), or some external url.
(No time to dig into that, too busy learning arabic and hebrew at the moment :)

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

Another issue is that our code doesn't expect the number of pages to change for PDF/DjVu/CBZ (i.e., paged media), so you'll still have to close and reload the document after making changes — but at least you'll see an instant font size preview now.

Making the number of pages adjustable on the fly might tie into #4382, though I'm not sure if reflowing into bite-sized pages can really be done sensibly without pulling the whole document through the wringer all at once. I've got a ton of ideas with regard reflow in principle, but very little motivation. K2pdfopt can quite easily do more cool stuff, perhaps most notably working on vectors instead of bitmaps for non-scanned documents.

Basically:

a. I seldom use reflow anyway. Fit to page, page width, and content width covers close to 99 % of my use cases. That'd be up to ten percentage points lower if landscape didn't exist, I suppose.
b. What remains is very satisfying to use reflow on in case of emergency or plain straightforward documents, which is most of them, but it's much quicker to do some touch-up of any sort on a proper computer. Particularly since results are better if you run pdfsandwich (wraps around unpaper and Tesseract) or Scan Tailor (cf. my script) first. And as soon as you're doing that, then it'd be silly not to run K2pdfopt afterward if desired.

The links thing should be easy to fix up though.

(No time to dig into that, too busy learning arabic and hebrew at the moment :)

I own a book called Cultural Connectives. It didn't exactly teach me the Arabic script (that is, I forgot most of the basic things it said), but it's beautifully designed.

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

@poire-z

In case you feel like fixing/enhancing on that reflowable mupdf front, it currently crashes when following footnotes link (just open any wikipedia epub), because of link_url being nil at that point:

Actually I can't reproduce. If I click footnote links they direct me to a page. (And yes, I am definitely in MuPDF. :-P) Could you please give me more detailed instructions and/or a problematic (Wikipedia) EPUB?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

Indeed, most of the links (including the inbook TOC) work, I actually can't reproduce it at the moment on the emulator, and if I enabled "Tap to follow links" on my Kobo.

As I usually don't have "Tap to follow links enabled", it happens when holding. I have a small patch to readerhighlight to actually follow link when holding:

     elseif self.selected_word then
+        -- if hold on a link, follow link rather than dict lookup
+        if self.selected_link then
+            self:clear()
+            self.ui.link:onGotoLink(self.selected_link)
+            self.selected_word = nil
+            self.selected_link = nil
+            return true
+        end
         self:lookup(self.selected_word, self.selected_link)

which was probably be the cause.

But you should be able to reproduce it by holding on a link, having the dict lookup, and hiting the "Follow link" button. This I can reproduce on the emulator :):

08/31/19-10:46:55 DEBUG External link:
./luajit: frontend/apps/reader/modules/readerlink.lua:510: attempt to index local 'link_url' (a nil value)
stack traceback:
 frontend/apps/reader/modules/readerlink.lua:510: in function 'onGotoLink'
 frontend/ui/widget/dictquicklookup.lua:461: in function 'callback'
 frontend/ui/widget/button.lua:207: in function 'handleEvent'
 frontend/ui/widget/container/inputcontainer.lua:259: in function 'handleEvent'
 frontend/ui/widget/container/widgetcontainer.lua:88: in function 'propagateEvent'
 frontend/ui/widget/container/widgetcontainer.lua:106: in function 'handleEvent'
 frontend/ui/widget/container/widgetcontainer.lua:88: in function 'propagateEvent'
 frontend/ui/widget/container/widgetcontainer.lua:106: in function 'handleEvent'

I get some other crash on my Kobo with https://fr.wikipedia.org/wiki/P%C3%A9tra saved as EPUB, tapping in the 2.2.1 section:

08/31/19-10:36:29 INFO  setting zoom mode to pagewidth
./luajit: frontend/document/koptinterface.lua:739: attempt to compare number with nil
stack traceback:
    frontend/document/koptinterface.lua:739: in function 'inside_box'
    frontend/document/koptinterface.lua:746: in function 'box_distance'
    frontend/document/koptinterface.lua:759: in function 'getWordBoxIndices'
    frontend/document/koptinterface.lua:795: in function 'getTextFromBoxes'
    frontend/document/koptinterface.lua:1055: in function 'getPageBoxesFromPositions'
    frontend/apps/reader/modules/readerview.lua:481: in function 'drawPageSavedHighlight'
    frontend/apps/reader/modules/readerview.lua:467: in function 'drawSavedHighlight'
    frontend/apps/reader/modules/readerview.lua:194: in function 'paintTo'
    frontend/ui/widget/container/inputcontainer.lua:85: in function 'paintTo'
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

EPUB is a red herring here. It's the internal link logic that's broken in that scenario, just the same for PDF. It arrives as link.link.page instead of just link.page. It must've worked at some point… last touched in #5282.

08/31/19-11:34:43 DEBUG link {
    ["lbox"] = {
        ["y"] = 180.54998779297,
        ["x"] = 141.16000366211,
        ["h"] = 31.580017089844,
        ["w"] = 120.4700012207
    },
    ["link"] = {
        ["y1"] = 201.13000488281,l
        ["x1"] = 250.63000488281,
        ["y0"] = 191.54998779297,
        ["x0"] = 152.16000366211,
        ["page"] = 19
    },
    ["pos"] = {
        ["page"] = 5,
        ["x"] = 194.42165242165,
        ["y"] = 194.84615384615,
        ["rotation"] = 0,
        ["zoom"] = 2.3557046979866
    }
}

Edit: It looks like there's a discrepancy between internal and external links.

08/31/19-11:45:04 DEBUG link {
    ["y1"] = 1743.9906005859,
    ["x1"] = 1285.7860107422,
    ["y0"] = 1705.9906005859,
    ["x0"] = 123.82199859619,
    ["uri"] = "https://www.hollywoodreporter.com/heat-vision/avengers-endgame-passes-avatar-become-no-1-film-all-time-1225121?utm_source=share&utm_medium=ios_app"
}
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

We can see that other code has some link.link stuff:

--- Highlights a linkbox if available and goes to it.
function ReaderLink:showLinkBox(link, allow_footnote_popup)
if link and link.lbox then -- pdfdocument
-- screen box that holds the link
local sbox = self.view:pageToScreenTransform(link.pos.page,
self.ui.document:nativeToPageRectTransform(link.pos.page, link.lbox))
if sbox then
UIManager:show(LinkBox:new{
box = sbox,
timeout = FOLLOW_LINK_TIMEOUT,
callback = function()
self:onGotoLink(link.link, false, allow_footnote_popup)
end
})
return true
end
elseif link and link.xpointer ~= "" then -- credocument
return self:onGotoLink(link, false, allow_footnote_popup)
end
end

function ReaderLink:onTap(_, ges)
if not isTapToFollowLinksOn() then return end
if self.ui.document.info.has_pages then
-- (footnote popup and larger tap area are for not
-- not supported with non-CreDocuments)
local link = self:getLinkFromGes(ges)
if link then
if link.link and link.link.uri and isTapIgnoreExternalLinksEnabled() then
return
end
return self:showLinkBox(link)
end
return
end

Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 31, 2019
[fix] Pass plain link to link:onGotoLink
Internal links carry more baggage than external ones.

See <koreader#5282 (comment)>.
Frenzie added a commit that referenced this pull request Aug 31, 2019
[fix] Pass plain link to link:onGotoLink (#5286)
Internal links carry more baggage than external ones.

See <#5282 (comment)>.
@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

I get some other crash on my Kobo with https://fr.wikipedia.org/wiki/P%C3%A9tra saved as EPUB, tapping in the 2.2.1 section:

I'm failing to reproduce. I tested tapping 2.2.1 in the Sommaire and both bootnotes in 2.2.1 itself.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

Well, I haven't managed to reproduce it on the emulator with a similarly looking view, but I get that crash on my kobo when I type in the red zone (not on the 2.2.1 title itself, in the text belonging to that section, sorry):
image

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

Weird. Did you create any highlights? Some pretty funny things happen there if you change the font size, though I haven't been able to trigger any crashes unless making the number of pages smaller than it was when adding the highlight.

08/31/19-18:10:55 DEBUG painting widget: ReaderUI
Warning in boxaSelectBySize: boxas is empty
Warning in boxaSelectBySize: boxas is empty
Error in boxaSort2d: boxas is empty
Error in pixaSort2dByIndex: naindex not defined
Error in pixaaFlattenToPixa: paa not defined
Error in pixaGetBoxa: pixa not defined
./luajit: frontend/document/koptinterface.lua:792: attempt to get length of local 'boxes' (a nil value)
stack traceback:
        frontend/document/koptinterface.lua:792: in function 'getTextFromBoxes'
        frontend/document/koptinterface.lua:1055: in function 'getPageBoxesFromPositions'
        frontend/apps/reader/modules/readerview.lua:481: in function 'drawPageSavedHighlight'
        frontend/apps/reader/modules/readerview.lua:467: in function 'drawSavedHighlight'
        frontend/apps/reader/modules/readerview.lua:194: in function 'paintTo'
        frontend/ui/widget/container/inputcontainer.lua:85: in function 'paintTo'
        frontend/ui/uimanager.lua:910: in function '_repaint'
        frontend/ui/uimanager.lua:1026: in function 'handleInput'
        frontend/ui/uimanager.lua:1098: in function 'run'
        ./reader.lua:243: in main chunk
        [C]: at 0x55a793510170
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

Nope. I've been asked when opening with MuPDF to delete my bookmarks, and so they were - so Bookmarks shows no bookmark.
But looking at the metadata.epub.lua, I see some trace of highlights:

    ["highlight"] = {
        [17] = {},
        [37] = {
            [1] = {
                ["datetime"] = "2019-07-17 23:15:22",
                ["drawer"] = "lighten",
                ["pos0"] = "/body/DocFragment/body/div/ul[4]/floatBox[2]/li/div/div[2]/p/text().61",
                ["pos1"] = "/body/DocFragment/body/div/ul[5]/floatBox[1]/li/div/div[2]/p/text().24",
                ["text"] = "taille.\
L'esplanade du Deir.\
Le Deir et son esplanade"
            }
        }
    },

Still can't reproduce the crash with the epub and .lua taken from my Kobo on the emulator...
In case it may help, here they are: koPetra.FR.zip

@Frenzie

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2019

I might take a look at not crashing on invalid highlights, but only superficially. This is more of a demo feature after all. ;-) Although with these new changes it does look deceptively usable.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2019

Might be just a matter of adding doc_settings:delSetting("highlight") in:

if #bookmarks >= 1 and
((provider.provider == "crengine" and type(bookmarks[1].page) == "number") or
(provider.provider == "mupdf" and type(bookmarks[1].page) == "string")) then
UIManager:show(ConfirmBox:new{
text = T(_("The document '%1' with bookmarks or highlights was previously opened with a different engine. To prevent issues, bookmarks need to be deleted before continuing."),
file),
ok_text = _("Delete"),
ok_callback = function()
doc_settings:delSetting("bookmarks")
doc_settings:close()
self:showReaderCoroutine(file, provider)
end,

Frenzie added a commit to Frenzie/koreader that referenced this pull request Aug 31, 2019
[fix] Prevent crash when no page boxes
Can occur with invalid page numbers, for example by changing the font size in a reflowable MuPDF document.

Discussion in <koreader#5282 (comment)>.
Frenzie added a commit that referenced this pull request Sep 1, 2019
[fix] Prevent crash when no page boxes (#5289)
Can occur with invalid page numbers, for example by changing the font size in a reflowable MuPDF document.

Discussion in <#5282 (comment)>.
Frenzie added a commit to Frenzie/koreader that referenced this pull request Sep 7, 2019
[fix] PdfDocument: Hash collision
A typo introduced in <koreader#5282>.

This might resolve <koreader#5323>, <koreader#5327>.
Frenzie added a commit that referenced this pull request Sep 7, 2019
[fix] PdfDocument: Hash collision (#5337)
A typo introduced in <#5282>.

This might resolve <#5323>, <#5327>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.