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

Fix LVDocView::getBookmark() which could be slow or wrong #313

Merged
merged 1 commit into from
Sep 29, 2019

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Sep 29, 2019

Fix issue noticed at koreader/koreader#5436 (comment).
Since 5967f72 (#312) and some change to be able to traverse RTL tables for text selection, LVDocView::getBookmark(), which is used to get the current page xpointer, could be slow on some pages, or wrong on others.
So, make it clear that, with createXPointer(lvPoint, direction) and elementFromPoint(lvPoint, direction), using direction=0 will check for x, while direction=1/-1 will not.

Wherever we are only interested in y, and provide a dummy x=0, we should use direction=1 or -1:

  • getPageDocumentRange() was already using 1/-1 exclusively.
  • getBookmark() now does too.
  • getNodeByPoint() uses 0 to find the exact node at x/y.
  • (Others LVDocView methods have not been updated nor checked, as they are not used by KOReader).

Previous related fixes on this topic in #195 #249 #299 ... I hope this doesn't break other things...

Since 5967f72 and some change to be able to traverse RTL tables
for text selection, LVDocView::getBookmark(), which is used to
get the current page xpointer, could be slow on some pages, or
wrong on others.
So, make it clear that, with createXPointer(lvPoint, direction)
and elementFromPoint(lvPoint, direction), using direction=0
will check for x, while direction=1/-1 will not.

Wherever we are only interested in y, and provide a dummy x=0,
we should use direction=1 or -1:
- getPageDocumentRange() was already using 1/-1 exclusively.
- getBookmark() now does too.
- getNodeByPoint() uses 0 to find the exact node at x/y.
- (Others LVDocView methods have not been updated nor checked,
  as they are not used by KOReader).
@poire-z
Copy link
Contributor Author

poire-z commented Sep 29, 2019

About this change:

-    if (!direction && pt.x != 0) {
+    if ( !direction ) {

which seemed like it was taking care of the issue by avoiding the check on x when we provide a pt.x=0:
As elementFromPoint is called recursively, with p->elementFromPoint( lvPoint( pt.x - fmt.getX()..., at some point when a child element has some left margin or border, our pt.x=0 would become no more 0, and this was no more helping, and we could be checking x.

@poire-z
Copy link
Contributor Author

poire-z commented Jan 11, 2020

Again another issue with LVDocView::getPageDocumentRange():
image
Current code peeks at top left and bottom right of page, to get the xpointers at these points, to get the the start and end xpointers of the page, so it can find for example links in that page.

That trick is fine with pure LTR text, but no more with RTL/bidi...
In the above screenshot, we miss "2007" in the page (but the "[7]" link is working) - and we don't get the links in the bottom row because the page ends with the most right word shown, which is really not the last word on that line but actually the first (and with bidi, it may be none).

Peeking also at top right and bottom left, and comparing xpointers to keep the most extrem ones, does not work (it gives the the other edge of the rightmost word...)
Might need to imagine another way to find these xpointers...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant