A bunch of fixes

[NiLuJe]

Some of 'em rather nasty (hi, crazy Document leak!).

---

This change is 

[poire-z]

No issue if we're viewing the cover via History of the same currently opened document in Reader?
(I see that this is not the case, but I would have expected DocumentRegistry:closeDocument() to itself closes the doc when refs becomes 0 :/)

[poire-z]

Oh, ok, Document:close() does not really close it if refs is not 0 :) So, the cleverness it there instead of here, but same thing in the end.

[NiLuJe]

Hopefully DocumentRegistry does its job and returns the cached instance ^^.

[NiLuJe]

Document:close only actually tears down the instance if the refcount drops to zero, so, provided everything works out, it's safe.

[NiLuJe]

(Whee, missed the second comment ^^).

Yeah, it's more of a case of non-intuitive naming & lack of documentation ;).

[poire-z]

Mhhh, not so sure about the ReaderFooter commit. That means we'd have a hard time doing a word lookup on the last line of the page... My reasoning for these overrides at the time in #5789.
I guess I'll just go uncheck "hold footer to skim" (probably wasn't there at the time I made these overrides.)
Anyway, feels a bit risky pushing it a few hours from 2021.05 - or you'll have 2021.05 postponed for a few more days :/

[poire-z]

Although I see that if you want "hold on footer to skim" to work as written on the tin (even if there's a word below), it has to be higher priority than ReaderHighlight.
But then, word lookup anywhere has more value than showing the skim - and the alternative to show the skim (gesture, menu) are easier than what would be needed to lookup the word there or highlight a sentence (scrolling).
As we don't assign anything to hold on corners, I think (if you want this) that we should (at least on new installations) not make "hold footer to skim" enabled by default - for consistency.

Anyway, nothing critical among the fixes of this PR, which could be fine for 2021.06.

[NiLuJe]

Mhhh, not so sure about the ReaderFooter commit. That means we'd have a hard time doing a word lookup on the last line of the page... My reasoning for these overrides at the time in #5789.

That works just fine now that we have an accurate Geom, which allows making the effective hold zone the footer itself, not the tap zone, meaning any hold outside the footer gets passed to ReaderHighlight.

(We couldn't do that originally because the footer's dimen was broken as all hell).

[NiLuJe]

As we don't assign anything to hold on corners, I think (if you want this) that we should (at least on new installations) not make "hold footer to skim" enabled by default - for consistency.

I actually have no idea what the default is, and no preference about that myself. I probably usually have it disabled in fact, as I had to enable it to test this ;p.

[NiLuJe]

Mhhh, not so sure about the ReaderFooter commit. That means we'd have a hard time doing a word lookup on the last line of the page... My reasoning for these overrides at the time in #5789.

That works just fine now that we have an accurate Geom, which allows making the effective hold zone the footer itself, not the tap zone, meaning any hold outside the footer get passed to ReaderHighlight.

(We couldn't do that originally because the footer's dimen was broken as all hell).

e.g., even an absolute worst-case scenario like that (thanks to reclaim height) works:

[Frenzie]

Last minute but tagged 2021.06?

[poire-z]

Little bit of discussion above, possibly a bit too risky, was changed from 2021.05 to 2021.06.

[Frenzie]

Alright, in that case I won't look at this PR (today, anyway).

[NiLuJe]

(Also, I broke my most favorite test in the world and didn't have an emu build ready to fix it ^^).

EDIT: Fixed it.

[NiLuJe]

Okay, I should be done playing with the tests now ;p.

[poire-z]

Fine with me (after the base bump).

(Wonder if the added self.ui and self.ui.document/doc_settings might hide future bugs :)

[Frenzie]

Use http://koreader.rocks/doc/modules/dbg.html#Dbg.dassert then. ;-)

[Frenzie]

@poire-z Could you comment on the relevant line? I'm not sure what you're referring to at a glance.

[poire-z]

This.

[Frenzie]

Thanks. So basically, something like:

Suggested change

	if ui and ui.document then
	dbg.dassert(ui ~= nil)
	if ui and ui.document then

[Frenzie]

How can UI be nil when the screensaver is activated? That doesn't seem to make sense.

[NiLuJe]

You won't have a ReaderUI instance in the FM, for example ;).

[poire-z]

Or this.

[poire-z]

Or that.

[poire-z]

The thing is that we witness edge/evil cases where some stuff might happen (via Dispatcher only?) while ReaderUI is half being born or half dying.
We can solve the issue using these kind of constructs - but the amount of such fixes in here feels low vs. the numerous self.ui.document we have :) Feels like: either fix them all - or fix none and fix the root cause.

[NiLuJe]

The thing is that we witness edge/evil cases where some stuff might happen (via Dispatcher only?) while ReaderUI is half being born or half dying.
We can solve the issue using these kind of constructs - but the amount of such fixes in here feels low vs. the numerous self.ui.document we have :) Feels like: either fix them all - or fix none and fix the root cause.

FWIW, those are all the instances where the ReaderUI instance pointer is accessed from outside (as opposed to inherited via an instance-time ui/dialog/parent/show_parent pointer).

(All of it is in 161a7b1)

The reasoning being stuff that inherits it usually has a clearer lifecycle, because it's instantiated and torn-down with that same ReaderUI instance (e.g., registerModule stuff).

[NiLuJe]

I can certainly wrap the extra hashkey checks in a debug assert if we're more comfortable with that, though?

[poire-z]

OK, I'm convinced :)

I can certainly wrap the extra hashkey checks in a debug assert if we're more comfortable with that, though?

No need I think, if it ends up being only History and ScreenSaver, that can obviously be used from FileManager.

[NiLuJe]

Reviewed 5 of 5 files at r1, 3 of 4 files at r2, 18 of 19 files at r3, 3 of 3 files at r4, 1 of 1 files at r5.
Dismissed @poire-z from 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @poire-z)

---

frontend/apps/filemanager/filemanagerhistory.lua, line 47 at r4 (raw file):

Previously, poire-z wrote…

Or this.

Done.

---

frontend/ui/screensaver.lua, line 306 at r4 (raw file):

Previously, poire-z wrote…

Or that.

Done.

[poire-z]

Got these WARN on my first launch after updating this morning:

05/20/21-12:56:53 INFO    opening took 0.615 seconds
05/20/21-12:56:54 INFO  GestureDetector:probeClockSource: Touch event timestamps appear to use CLOCK_MONOTONIC
05/20/21-12:56:55 WARN  Tried to close a document with *multiple* remaining hot references
ffi.load: libs/liblept.so.5
ffi.load: libs/libk2pdfopt.so.2
ffi.load: libs/liblodepng.so
ffi.load: libs/libturbojpeg.so
ffi.load: libs/libgif.so.7
05/20/21-12:57:17 WARN  Tried to close an already closed document
05/20/21-12:57:21 INFO  no dialog left to show
05/20/21-12:57:21 INFO  quitting uimanager

Don't remember what I did (gestures, taps?), but I can't reproduce...

OK.
I get the 2nd one when Reader> History (via a gesture) > View full size cover (but only once for the 3 covers I looked at).

Trying that again, I get them both again (some View full size covers from History) - plus a crash :/ :

05/20/21-14:07:05 INFO    opening took 1.376 seconds
05/20/21-14:07:06 INFO  GestureDetector:probeClockSource: Touch event timestamps appear to use CLOCK_MONOTONIC
05/20/21-14:07:31 WARN  Tried to close a document with *multiple* remaining hot references
05/20/21-14:08:00 WARN  Tried to close an already closed document
./luajit: frontend/document/credocument.lua:1483: attempt to index field '_tag_list_call_cache' (a nil value)
stack traceback:
    frontend/document/credocument.lua:1483: in function '_callCacheSetCurrentTag'
    frontend/document/credocument.lua:1733: in function 'gotoPage'
    frontend/document/credocument.lua:793: in function 'drawCurrentViewByPage'

[NiLuJe]

Ah, that might explain all the gesture weirdness we've seen lately...

I'll see what I can suss out (because that works fine from Menus, FWIW) :}.

[NiLuJe]

Okay, the warning is normal: if you view the cover of the currently open'ed book, that's a "cache" hit in DocumentRegistry (so, refcnt == 2 on open, and 1 on close => warn).

The crash is sneakier, it's because the engine's close method is called before the refcnt check (since it's handled by the base class's close), and it may destroy critical engine-specific data (here, the buffer and the call cache).

[hius07]

Shouldn't be or instead of and not?

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 592 to 605 in bfda4bf

	if self.settings.progress_bar_position ~= "alongside" and not self.settings.disable_progress_bar then
	self.horizontal_group = HorizontalGroup:new{
	margin_span,
	self.text_container,
	margin_span,
	}
	else
	self.horizontal_group = HorizontalGroup:new{
	margin_span,
	self.progress_bar,
	self.text_container,
	margin_span,
	}
	end

Now, if the progress bar is disabled, horizontal_group includes progress_bar with zero width.

[NiLuJe]

@hius07: Logic should be unchanged (compared to before the PR). The fact that the layout code explicit handles disable_progress_bar with a zero width makes me thing this is best left untouched ;).

(I mean, feel free to experiment if you want, but unless you can actively break it, meh' ;)).

[hius07]

May I ask one more question. What is the difference in getting margins here

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 2062 to 2063 in bfda4bf

	local margins = self.ui.document:getPageMargins()
	self.settings.progress_margin_width = math.floor((margins.left + margins.right)/2)

and here

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 2075 to 2078 in bfda4bf

	local h_margins = config:readSetting("copt_h_page_margins")
	or G_reader_settings:readSetting("copt_h_page_margins")
	or DCREREADER_CONFIG_H_MARGIN_SIZES_MEDIUM
	self.book_margins_footer_width = math.floor((h_margins[1] + h_margins[2])/2)

[hius07]

I mean, can we always ask for self.ui.document:getPageMargins(), instead of passing aruments such as the second piece of code above or

koreader/frontend/apps/reader/modules/readerfooter.lua

Lines 2282 to 2288 in bfda4bf

	function ReaderFooter:onSetPageHorizMargins(h_margins)
	self.book_margins_footer_width = math.floor((h_margins[1] + h_margins[2])/2)
	if self.settings.progress_margin then
	self.settings.progress_margin_width = Screen:scaleBySize(self.book_margins_footer_width)
	self:refreshFooter(true)
	end
	end

[poire-z]

The 2nd piece of code above is in ReaderFooter:onReadSettings(), at the time every setting is read from docsettings and fed to crengine. crengine may not have yet rendered things, so it's possible at that time that document:getPageMargins() has either get yet not value, or has values not yet fully checked/usable.

For :onSetPageHorizMargins(), ReaderFooter and ReaderTypeset got them. ReaderTypeset is the one that will give it to crengine. And it looks that with the order of modules, ReaderFooter will get the even before ReaderTypeset - so using document:getPageMargins() would give back the previous/older margins values.