Optimization of page turn command and picking of the last commits of the koreader.

[virxkane]

• The command for switching to the next/previous page has been optimized due to the optional excluding of the element search loop in LVDocView::getPageDocumentRange() and LVDocView::getBookmark(). The execution time for this command has been significantly reduced, before this fix, in some cases it reached 6 seconds, now it does not exceed 100 ms on the same device.
  See https://4pda.ru/forum/index.php?showtopic=191747&view=findpost&p=99147082
  Based on Merged with KOReader fork of crengine #125 (comment)
• Fixed compile error in Tools/Fb2Linux/fb2v.cpp
• (koreader) TextLang: better linebreaks at em-dash (EN/ES/FR) Try to improve em-dash linebreaks koreader/crengine#365
• (koreader) FB2: also look for cover in Various upstream and FB2 fixes koreader/crengine#366
• (koreader) FB2: fix cover image colors Various upstream and FB2 fixes koreader/crengine#366
• (koreader) TextLang: increase _lb_props static array size TextLang: increase _lb_props static array size koreader/crengine#368

[virxkane]

@poire-z I guess the commit 7737527 won't break anything in the koreader.
Based on #125 (comment)

[poire-z]

(sorry for the delay in responding...)

I guess the commit 7737527 won't break anything in the koreader.

It won't break anything, as we won't provide precise=false.
But I'm not sure it is right to skip these loops, even if you don't want anything precise.
The issue was that previously, and now with your precise=false, these functions (at least getBookmark(), which is the only one I noticed issue with) could return a null xpointer/range.
With KOReader, the issue was that you browse pages, go on a page with some blank margin at top, and then exit KOReader (or switch to another document, home...). Then opening again this document, you would be on page 1 because your previous last page got a null wpointer/bookmark.
(We also use that when bookmarking a page, so, our bookmark icon that you just set on a page, might not be there when you browse one page forward and then back.)
Dunno how these might affect CoolReader and it's last-page/bookmarks.

That slowness happens only in scroll mode ? And not at all in page mode?
Does the slowness in turning pages happen also with KOReader (I think I remember we tried that and it did not, I might be wrong).

About the textlang patch, was there really a need for KO_LIBUNIBREAK_PATCH (and that little divergence introduced) ? (btw, you forgot it in textlang.h, althought it wouldn't cause any issue as long as it's not called).

[virxkane]

But I'm not sure it is right to skip these loops, even if you don't want anything precise.

Called with argument 'false' only in DocView.getPositionProps() which is used only for page turn, text scroll, show TOC dialog, show book info, update current position status. It may also be necessary to add this argument 'precise' for functions higher up in the call stack, so that getBookmark() and getPageDocumentRange() was called with the 'precise' argument 'false' only when turning pages and text scrolling.
Btw in this function() it's safe if getBookmark() returns null:

coolreader/android/jni/docview.cpp

Lines 1696 to 1711 in 249a068

	ldomXPointer bm;
	bool useCurPos = false; // use current Y position for scroll view mode
	p->_docview->checkPos();
	if ( !str.empty() ) {
	bm = p->_docview->getDocument()->createXPointer(str);
	} else {
	useCurPos = p->_docview->getViewMode()==DVM_SCROLL;
	if ( !useCurPos ) {
	bm = p->_docview->getBookmark(false);
	if ( bm.isNull() ) {
	CRLog::error("getPositionPropsInternal: Cannot get current position bookmark");
	}
	}
	}
	CRObjectAccessor v(_env, obj);
	lvPoint pt = !bm.isNull() ? bm.toPoint() : lvPoint(0, p->_docview->GetPos());

With KOReader, the issue was that you browse pages, go on a page with some blank margin at top, and then exit KOReader (or switch to another document, home...). Then opening again this document, you would be on page 1 because your previous last page got a null wpointer/bookmark.

So I hope it's not affected to CoolReader.

Dunno how these might affect CoolReader and it's last-page/bookmarks.

We should check then. How can I do? Can you upload specific file for testing?

That slowness happens only in scroll mode ? And not at all in page mode?
Does the slowness in turning pages happen also with KOReader (I think I remember we tried that and it did not, I might be wrong).

Mostly, before: ScrollViewAnimation preparation 3739ms, after: 63ms. But page mode also affected: before: PageViewAnimation preparation 125 ms, after 63 ms.

About the textlang patch, was there really a need for KO_LIBUNIBREAK_PATCH (and that little divergence introduced) ?

I don't see any other solution until this patch is included in the libunibreak upstream: desktop build of CoolReader use system library libunibreak, and without this patch we will get a compilation error. Those we will be forced to stop using the library installed on the system.

(btw, you forgot it in textlang.h, althought it wouldn't cause any issue as long as it's not called).

I don't think there was anything to be added there. I add corresponding option into Android project file:

coolreader/android/app/CMakeLists.txt

Line 36 in 249a068

add_definitions(-DKO_LIBUNIBREAK_PATCH=1) # patch "add_lb_get_char_class.patch" for libunibreak from koreader

[poire-z]

desktop build of CoolReader use system library libunibreak, and without this patch we will get a compilation error

Oh, ok, I didn't know that. Allright then.
(So, desktop users won't have better dash with english. Can't you build with the libunibreak/harfbuzz/fribidi you ship anyway ? Trusting system version might have CR use older incompatible versions and cause crash you'll have a hard time understanding/debugging...)
(I don't plan with proposing that patch to upstream.)

I don't think there was anything to be added there.

Oh, allright again, these are not defined in textlang.h. I got confused :)

We should check then. How can I do? Can you upload specific file for testing?

I think I triggered that bit of code with the Petra.AR as mentionned at #125 (comment) - when a float is split on 2 pages. I think I might have tweaked the CSS (all float: & clear: left), so here is one where the chapter 7 looks the same: Petra2.AR.zip (you might want to change font size if the cut does not happen because text and float would luckily just align).

Also may be when there is some margin-top on some element at top (like chapter title, bordered images in Wikipedia EPUBs when the border is at top of page), but may be only in legacy rendering mode (as we may drop that top margin in enhanced rendering).

Called with argument 'false' only in DocView.getPositionProps() which is used only for page turn, text scroll, show TOC dialog, show book info, update current position status. It may also be necessary to add this argument 'precise' for functions higher up in the call stack, so that getBookmark() and getPageDocumentRange() was called with the 'precise' argument 'false' only when turning pages and text scrolling.

But then, why are you calling these functions if you are fine with them returing NULL ? :) May be you just donc care about the return values, and these calls could be avoided in these context.

Quick look at your first commit about getPositionPropsInternal():

• lvPoint pt = !bm.isNull() ? bm.toPoint() : lvPoint(0, p->_docview->GetPos()); - may be just always use the 2nd alternative if you're fine with it whem bm.isNull()
• about getting getRangeText() to store(?) it in a pageField text: why/when is it used? Is it needed to be saved on each page turn ? Or just when bookmarking. In which case yes, the precise argument might be given to getPositionPropsInternal(precise/detailed = true/false) and just don't get text range, and use lvPoint(0, p->_docview->GetPos()); when not required ?

[virxkane]

I think I triggered that bit of code with the Petra.AR as mentionned at #125 (comment) - when a float is split on 2 pages. I think I might have tweaked the CSS (all float: & clear: left), so here is one where the chapter 7 looks the same: Petra2.AR.zip (you might want to change font size if the cut does not happen because text and float would luckily just align).

Thanks, I will test how the time will be.

But then, why are you calling these functions if you are fine with them returing NULL ? :) May be you just donc care about the return values, and these calls could be avoided in these context.

Quick look at your first commit about getPositionPropsInternal():

• lvPoint pt = !bm.isNull() ? bm.toPoint() : lvPoint(0, p->_docview->GetPos()); - may be just always use the 2nd alternative if you're fine with it whem bm.isNull()

I didn't write it. I can't even guess where this will lead :)

https://github.com/buggins/coolreader/blame/249a06823be01cbc69613eefcb55381513785a3a/android/jni/docview.cpp#L1711

• about getting getRangeText() to store(?) it in a pageField text: why/when is it used? Is it needed to be saved on each page turn ? Or just when bookmarking. In which case yes, the precise argument might be given to getPositionPropsInternal(precise/detailed = true/false) and just don't get text range, and use lvPoint(0, p->_docview->GetPos()); when not required ?

May be it's a good idea.
To be honest, I have no desire to understand this ancient spaghetti code.