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

EPUB links: show footnotes in popup, larger tap area #4261

Merged
merged 4 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@poire-z
Contributor

poire-z commented Oct 9, 2018

Adds new options to the Links> submenu, for now only available and used with CRE documents (*).

image

The Show footnotes in popup is both in the Tap and the Swipe sub-sections, as one may want it for Tap but still follow with Swipe, and some others may have disabled Tap, but still want to see footnotes with Swipe.
The Show more links as footnotes is mostly for testing and loosening the footnote detection algorithm, and see how it would behave with glossary-like links and inter glossary terms links.
(Please have a look at the settings names, I'm not really satisfied with the link_prefer_foonote name, if we keep that item.)

Fixed distance computation from gesture position to link by using segments.
Code for detecting if a link is a footnote is in cre.cpp, and tweakable a bit with flags in ReaderLink:showAsFoonotePopup().

Footnotes' HTML content is displayed by a new FootnoteWidget, which uses MuPDF for its rendering.
From it, swipe south or tap outside to close, swipe to the left to follow the original link and jump to the footnote location in the book.

image

image

With larger margins and a smaller font size:
image

Original discussion in #3054 (comment). Closes #3054.

Will need a base bump for koreader/koreader-base#741.

(*) only available and used with CRE documents : I had a try with paper.pdf before PR'ing this, and it was too annoying to have "allow larger tap area" enabled: it mostly works, but as you most often don't see what is a link or not, and that there is no real visual feedback that your tap leads you to a link and not to the next page (provided with EPUB with the usual underlining of <A> and the little markers on the side when following a link) , I decided to just disable them. It may need a little more thinking if that could be useful with PDF. Footnote detection and popups are of course hardly possible with PDF.

EPUB links: show footnotes in popup, larger tap area
Adds new options to the Links> submenu, for now only
available and used with CRE documents.
- Allow larger tap area around links
- Ignore external links
- Show footnotes in popup
- Show more links as footnotes

(This last item is mostly for testing and loosening the
footnote detection algorithm, and see how it would behave
with glossary-like links and inter glossary terms links.)

Fix distance computation from gesture position to link by
using segments.
Code for detecting if a link is a footnote is in cre.cpp, and
tweakable a bit with flags in ReaderLink:showAsFoonotePopup().

Footnotes HTML content is displayed by a new FootnoteWidget,
which uses MuPDF for its rendering.
From it, swipe south or tap outside to close, swipe to the left
to follow the original link and jump to the footnote location
in the book.
@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 9, 2018

For the footnote detection algorithms, I used ideas from what Kobo and Calibre do.
I had the following notes as comments in the code, that I removed as it was quite big. Putting them here just in case.

For reference, Kobo information and conditions for showing links as popup:
https://github.com/kobolabs/epub-spec#footnotesendnotes-are-fully-supported-across-kobo-platforms

  - The link references a location in the ePub and also references a specific node within the HTML
  - The content in the node is nine characters or more once stripped of the HTML tags
  - The node being linked to is less than or equal to 5000 characters
  - The location being linked to comes after the location being linked from

Calibre has its own heuristics to determine if a link is to a footnote or not, and what to gather around the footnote target as the footnote content to display. Nearly same logic, implemented in python and in coffeescript:
https://github.com/kovidgoyal/calibre/blob/master/src/pyj/read_book/footnotes.pyj
https://github.com/kovidgoyal/calibre/blob/master/src/calibre/ebooks/oeb/display/extract.coffee

From what I understand:

To guess if a <a href> node is a link to a FOOTNOTE to display in a popup, or
a REGULAR link to follow:
  a py) if role="" attribute (space separated list of "roles") contains:
         doc-noteref => FOOTNOTE, STOP.
         doc-link    => REGULAR, STOP.
  a cs) if epub:type="" (or any *:type="" with a set of possible values) attribute
        is once lowercased:
         noteref => FOOTNOTE, STOP.
         link    => REGULAR, STOP.
  b) check for node and its first 2 parents (stop checking when one is not display: inline)
     if its vertical-align: is any of: sub super top bottom.
     If found => FOOTNOTE, STOP.
     (a link INSIDE a <sup> is so considered to be to a footnote)
  c) check if node has all its text nodes trimmed empty (so, only spaces),
     AND the single non-text child AND that single element node:
       has display: inline AND has vertical-align: any of "sub super top bottom"
       => FOOTNOTE, STOP.
     (a standalone <sup> INSIDE the link makes it considered to be to a footnote)
  d) "An <a href= id=> link that is linked back from some other file in the spine,
        most likely an endnote. We exclude links that are the only content of
        their parent block tag, as these are not likely to be endnotes"
     if the A itself has an ID= or NAME= attribute, if this ID is a target from
     some other links in the book (from calibre book.manifest.link_to_map[current_file][current_id]
     that lists all the epub files that have a link to this id)
     if more than one file (if one single file, that file should not be current one):
       - get first parent block container
       - if none, or if it is BODY => REGULAR, STOP.
       - if this blockcontainer -> getText() is empty => REGULAR, STOP.
       - if our A->getText() is empty => REGULAR, STOP.
       - if A->getText() == blockcontainer->getText() => REGULAR, STOP.
       - otherwise => FOOTNOTE, STOP.
  e) => REGULAR, STOP.

To gather footnote content, from target node with target id:
  a) go up to parent which is either P DIV LI TD H1 H2 H2 H3 H4 H5 H6 BODY
     or have display: block list-item table-cell table
  b) get the full tree of parents only (ignoring siblings), and set list-display-style: none
     to any OL>LI as the ordered number value would be wrong with siblings removed
  c py) if role="" attribute (space separated list of "roles") contains any of
         doc-note doc-footnote doc-rearnote
     stop there and use that node.
  c cs) if epub:type="" (or any *:type="" with a set of possible values) attribute
        is one of:
         note footnote rearnote
     stop there and use that node.
  d) consider all elements after (even if not inside) the found parent, and stop when:
     - (before) a node with page-break-before
     - (after) a node with page-break-after
     - a node with a ID= attribute is found AND that ID value is among the
       target IDs in "book.manifest.link_to_map?[self.name]" (which lists all
       the targeted IDs in this EPUB fragment) (so, probably another footnote) AND
       that ID value is not the targeted ID AND this node parent block container
       is different than the one we used from (a)
@@ -71,6 +71,22 @@ local function isTapToFollowLinksOn()
return not G_reader_settings:isFalse("tap_to_follow_links")
end
local function isLargerTapAreaToFollowLinksEnabled()
return G_reader_settings:readSetting("larger_tap_area_to_follow_links") == true

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

Instead of readSetting(), use isTrue()? :-)

This comment has been minimized.

@poire-z

poire-z Oct 9, 2018

Contributor

Cut and pasted from its siblings :) but indeed.
(I often wondered why there are these functions in that module, but hardly in others - I guess it helps avoiding typo in the setting name.)

@@ -83,6 +99,10 @@ local function isSwipeToFollowNearestLinkEnabled()
return G_reader_settings:readSetting("swipe_to_follow_nearest_link") == true
end
local function isSwipeLinkFoonotePopupEnabled()
return G_reader_settings:readSetting("swipe_link_foonote_popup") == true

This comment has been minimized.

@Frenzie

This comment has been minimized.

@poire-z

poire-z Oct 9, 2018

Contributor

There are tons of them, and I've been having them under my nose for days !

G_reader_settings:saveSetting("larger_tap_area_to_follow_links",
not isLargerTapAreaToFollowLinksEnabled())
end,
help_text = _([[Extends the tap area around internal links. Useful with a small font where taping on small footnote links may be tedious.]]),

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

tapping

G_reader_settings:saveSetting("tap_ignore_external_links",
not isTapIgnoreExternalLinksEnabled())
end,
help_text = _([[Ignore tap on external links. Useful with Wikipedia EPUBs full of them to make page turning easier.

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

Ignore taps on external links. Useful with Wikipedia EPUBs to make page turning easier.

-- to a longer Wikipedia article name link.
-- (1/16 of screen width looks fine - should this use another
-- metric like screen DPI ?)
max_distance = math.min(Screen:getHeight(), Screen:getWidth()) * 1/16

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

Yeah, I think DPI would probably be what we want here. Normally something like Screen:scaleBySize(10), but in this case possibly just using DPI because really we're just looking for something like 20-30 mm extra anchor padding.

@@ -563,6 +713,29 @@ function ReaderLink:onGoToPageLink(ges, use_page_first_link)
-- ["start_y"] = 1201
-- ["a_xpointer"] = "/body/DocFragment/body/div/p[12]/sup[3]/a[3].0",
-- },
-- and when segments requested (example for a multi-lines link):
-- [3] = {
-- ["section"] = "#_doc_fragment_0_ Développement_de_lâéconomie_marchande",

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

Is the weird encoding on développement and économie supposed to be there, just a GH display artifact, or …? :-)

I just find it distracting, sorry. :-D

This comment has been minimized.

@poire-z

poire-z Oct 9, 2018

Contributor

I'll fix that, it's a mix of latin1 and utf8 cut & pasted from koreader debug log output in my latin1-only terminal , and GH doing what it can with that :)

selected_link.link_y = segments_max_y
end
else
-- Before "segments" were available, we did that:

This comment has been minimized.

@Frenzie
-- error: css syntax error: unterminated comment.
-- So, HTML tags in comments are written upppercase (eg: <li> => LI)
-- Independant string for @page, so we can T() it individually,

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

independent

bottom = Screen:scaleBySize(10),
},
width = Screen:getWidth(),
height = Screen:getHeight() * 1/3, -- will be decreased when content is smaller

This comment has been minimized.

@Frenzie

Frenzie Oct 9, 2018

Member

Shouldn't this be rounded down or something?

poire-z added some commits Oct 10, 2018

highlights: also use segments when checking Tap
Otherwise, taping on a now-highlighted brackets wouldn't bring
the Delete|Edit menu.
@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 10, 2018

I let in 4 logger.info(), just so I/we get some logs for some time to have some info when footnotes work/don't work. That may pollute crash.log for people who will be using this feature, so we should remember to remove them in a few weeks.

10/10/18-13:11:43 DEBUG Checking if link is to a footnote: 57206 /body/DocFragment/body/div[1]/p[7]/sup[1]/a.0 #_doc_fragment_0_ cite_note-2
10/10/18-13:11:43 INFO  is a footnote: source has only vertical-align: super/sub/top/bottom
10/10/18-13:11:43 INFO    not extended because: node with 'id=' attr met

10/10/18-13:09:02 DEBUG Checking if link is to a footnote: 57175 /body/DocFragment[1131]/body/div/p[4]/a[4].0 #_doc_fragment_1649_ joug
10/10/18-13:09:02 INFO  is a footnote: no decision, default to be a footnote
10/10/18-13:09:02 INFO    extended until: node with 'id=' attr met
10/10/18-13:09:02 INFO  /body/DocFragment[1650]/body/div/div[2]/autoBoxing[1].0
10/10/18-13:09:02 INFO  /body/DocFragment[1650]/body/div/div[2]/autoBoxing[4]/text()[3].37

@poire-z poire-z merged commit f9086a2 into koreader:master Oct 10, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:footnotes branch Oct 10, 2018

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Maybe I've missed something but I can't get any popup footnotes on my KA1.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

Any line in your crash.log similar to the one i mentionned above, or to this:
10/10/18-13:09:02 INFO not a footnote: <reason why>

If no such log, either it's not enabled in the menu, or you're reading a PDF, or I missed something :)
(I haven't tested today's nightly, anyone can confirm they have footnotes working?)

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Yes, I found a lot of these: INFO not a footnote: no decision made, default to not a footnote

Epub & enabled.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

OK, so you don't have the super obvious candidates for footnotes: your links are not in subscript or superscript vertical position, nor their text is just a few numbers or letters. Is that the case?
If yes, you can enable the other option "Show more links as footnotes", and they should work (with the risk that a link to a chapter will show the whole chapter as a footnote :)

But try without this option on other books where you may have footnotes that match the "safe" footnote criteria described above.

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Yes! Now they work.

Thanks.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

But please tell me if you have other books with the criteria I used to detect "safe" footnotes.
In my french books, and a few english, these criteria are there in 90% cases.
But may be it's not the case in other languages/cultures, and we may need to consider other criteria.

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

But please tell me if you have other books with the criteria I used to detect "safe" footnotes.
In my french books, and a few english, these criteria are there in 90% cases.
But may be it's not the case in other languages/cultures, and we may need to consider other criteria.

I can't find any.
I've tried maybe 50 books so far. I need "Show more links as footnotes" for them all.

English (and some swedish). Not obscure books, but kind of Amazon bestsellers in history, biographies etc.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

I'm curious to see how these footnote links look. Could you post a few screenshots from different books if you can't share any books?

edit: They work for me in the book you posted in #3731 (comment)

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Here is one book:

By the way, I have found one that works now.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

They work for me. Note 3 in chapter 1) Nation:
image

(Just to be sure: what you mean by not working = the link is followed to some other footnote page "in book"? And not: the tap does not have any effect, or take you to next/prev page? or a small 1 line empty footnote is shown?)

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Douglas Murray book: not working (tested note 1 & 2 in chapter 1)

David Reynolds book: not working (tried note 3 chapter 1)

Not working = meaning as it was before, i.e. link followed to footnote page in the back of the book.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

The all work for me (with Show more disabled) :|
What about with some Wikipedia save as EPUB?

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

What about with some Wikipedia save as EPUB?

Never used that and I have no wi-fi right now.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

Just for testing, if you can (backup it before if needed) Purge .sdr for one of these 2 books that work for me, and check you have no Style tweak and floating punctuation disabled, so we are more alike.

Here's one Wikipedia epub: LaTeX.EN.epub.zip

I'd like some other people to check how these Reynolds a Murray book do for them...

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

Wikipedia: not working

I disabled my style tweaks and now the popup footnotes work.
Didn't know I can't have style tweaks anymore.

EDIT
Sorry, I have to correct myself.
I disabled the style tweaks for one book (without saving this setting) and the footnotes worked.
And then I opened other books, without disable my style tweaks settings for them, and the footnotes worked for them too.
Anyway, now it seems to work for me too.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

They should work with Style tweaks. There may be just some tweaks that cause your problem.
Could you pinpoint it to which tweak you had enabled that cause the problem and that is enough to disable?
A tweak from KOReader or do you have some user tweaks of your own? (because I don't see one in KOReader tweaks that should interfere with footnotes, including the Smaller sub- and superscript...

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

As I wrote in my edit: a temporary disabling of the tweaks and then let the saved enabling stay did the trick.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 11, 2018

Strange... I see no explanation for that behaviour and for that "fix" :| But ok, all is good then :)

@gerhaher

This comment has been minimized.

gerhaher commented Oct 11, 2018

By the way this is my style tweaks:

Avoid blank page on chapter start
Ignore publisher font families
Ignore publisher font sizes

@gerhaher

This comment has been minimized.

gerhaher commented Oct 12, 2018

Hi again!

I'm so sorry, but I gave false information yesterday. Today I found out that it is still don't working.
I don't know what happened, I must have accidentally enabled "Show more...".
So forget about the style tweak thing.

However, after a lot of testing I found another suspect: the full Progress bar

With full progress bar: not working
With mini progress bar: working

I don't know if it make sense for you but in any case I can reproduce it on both my KA1 and my Kobo Aura (with a new, clean KOreader installation)

On a side note: it seems there is an issue with setting full progress bar to default mode. It doesn't stick for me

@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 12, 2018

Just the footnotes or not the source view either?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

With full progress bar: not working

That makes more sense :)
I never test anything with the full progress bar... and there are a few things that are messed up: footnotes, but also the little black markers on the side when following links and back that have a wrong y coordinate position.

I'll have a look, might be it's just a matter of shifting y coordinate in a few places by the additional height used by the top status bar.

View HTML seems to work fine.

On a side note: it seems there is an issue with setting full progress bar to default mode. It doesn't stick for me

I think the setting is stored "per book", so it should stick "per book". If you hold on the button, it's set as a default, but will be used only for new books (for old books, the "per book" saved setting will still be used).

@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 12, 2018

@gerhaher

This comment has been minimized.

gerhaher commented Oct 12, 2018

I think the setting is stored "per book", so it should stick "per book". If you hold on the button, it's set as a default, but will be used only for new books (for old books, the "per book" saved setting will still be used).

I don't understand what you mean here.

This is how it works for me right now:
If I long tap "full" and then "set default", what's happening is that when I open a new, formerly unopened book - yes the "full" is greyish when I check, but there is no full progress bar on the page.

I have always had full progress bar as default before without any problem.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

OK, right, I didn't test. It seems to work, it is just not applied on new book first load (if you quit and re-open that now-not-new book, the full status bar is now displayed). Right?

@gerhaher

This comment has been minimized.

gerhaher commented Oct 12, 2018

Yeah.
It wasn't like this before.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

It wasn't like this before.

Looks like it has been like that forever.
No code is using the default value (set with Hold) saved in settings.reader.lua, only the value (set with Tap) saved in the book.sdr/metadata.epub.lua, OR the default value from default.lua or defalut.persistent.lua:

koreader/defaults.lua

Lines 133 to 136 in 76a3d20

-- crereader progress bar
-- 0 for top "full" progress bar
-- 1 for bottom "mini" progress bar
DCREREADER_PROGRESS_BAR = 1

May be, with trying with a new/clean installation, you lost a change to have full progress bar as default you previously made there?

(It's the same for view mode page/scroll: the default value set with Hold is not used...)

@gerhaher

This comment has been minimized.

gerhaher commented Oct 12, 2018

May be, with trying with a new/clean installation, you lost a change to have full progress bar as default you previously made there?

Yeah, could be.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 12, 2018

No code is using the default value (set with Hold) saved in settings.reader.lua, only the value (set with Tap) saved in the book.sdr/metadata.epub.lua, OR the default value from default.lua or defalut.persistent.lua:

Really? Hm, in the event that I'm bored on the train tomorrow I might take a look at that. I'll probably just be reading though. ;-)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 12, 2018

I'm sure and hope you'll be reading :), so I fixed that in #4268 (if I didn't mess it up, I really don't like modifying that kind of old code and wondering which module should do what when they all do a bit...)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Oct 12, 2018

Well, it's just that when you're on the train for 3 hours you might do something else for variation, or just because switching trains around Amsterdam got you out of the zone. Which reminds me, I need to pull in the latest stuff.

Anyway, I won't be looking at that then.

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented Oct 15, 2018

@poire-z : Would it be possible to disable justification just for the popup ?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Oct 15, 2018

You could try removing this line:


Personally, I find it really ugly, when the book content in justified (and there's no way to know how the book content is, to use the same for the footnote).
If there's really a need, and you have already disabled text justification for the dictionary results (in the Dict settings menu), we could re-use that setting (just to not introduce a new one) for that footnote popup.

@cramoisi

This comment has been minimized.

Contributor

cramoisi commented Oct 15, 2018

@poire-z : Thanks (but i was just asking :) I won't do it ahah)

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