ensure pan zoom mode on document open

[hugleo]

When we open the document for the first time, the pan positions are not being applied. If I use the bottom-to-top mode we should see the bottom first but the top is being shown instead.

ReadSettings is calling PageUpdate, but for some reason, some events are delayed, for example, at least onInitScrollPageStates is only triggered when we pop out ReadSettings event. These events are needed whatever settings is doing for PageUpdate do properly for pan zoom.

---

This change is 

[poire-z]

I don't think this (ReaderUI:init()) is really the place to do that. Doing it here will have it done also with ReaderRolling (cre/epub documents), which has no issue and so does not need it (and it might cause issues if this provokes too much stuff in readerrolling too early before anything else is set up.

It should probably be done in ReaderPaging only, and probably way later than just after onReadSettings maybe not, it seems ReaderPaging already does that very early (probably because it's less complex that crengine in the amount of events and their proper ordering):

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

Lines 189 to 191 in f836f6a

	function ReaderPaging:onReadSettings(config)
	self.page_positions = config:readSetting("page_positions") or {}
	self:_gotoPage(config:readSetting("last_page") or 1)

and this _gotoPage seems to already send that event (?):

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

Lines 1103 to 1120 in f836f6a

	function ReaderPaging:_gotoPage(number, orig_mode)
	if number == self.current_page or not number then
	-- update footer even if we stay on the same page (like when
	-- viewing the bottom part of a page from a top part view)
	self.view.footer:onUpdateFooter(self.view.footer_visible)
	return true
	end
	if number > self.number_of_pages then
	logger.warn("page number too high: "..number.."!")
	number = self.number_of_pages
	elseif number < 1 then
	logger.warn("page number too low: "..number.."!")
	number = 1
	end
	-- this is an event to allow other controllers to be aware of this change
	self.ui:handleEvent(Event:new("PageUpdate", number, orig_mode))
	return true
	end

[hugleo]

Didn't work if done it inside of onReadSettings after the last line. I even try to call _gotoPage two times.
I even try the ReaderPaging:onGotoPage variant.
Maybe some kind of event dependency queued to be executed after onReadSettings? onInitScrollPageStates for example I see reach the logs.

[poire-z]

Probably. Other modules will also process onReadSettings, and may be it depends on some stuff done by omdules who does it after.
Would be nice to figure out why it does not work and understand what's needed.
There is another event/handler ReaderPaging:onReaderReady() that is called way later, that oculd be a last resort solution :)

[hugleo]

Probably. Other modules will also process onReadSettings, and may be it depends on some stuff done by omdules who does it after. Would be nice to figure out why it does not work and understand what's needed.

I'll try it when find more time, another fifty percent there we go...

[hugleo]

Found the reason.

The settings that are needed are here: https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerzooming.lua#L267

The events order:

02/02/24-23:32:42 WARN ReaderView onPageUpdate end
02/02/24-23:32:42 WARN ReaderPaging onPageUpdate end
02/02/24-23:32:42 WARN ReaderZooming onPageUpdate end
02/02/24-23:32:42 WARN ReaderPaging onReadSettings end
02/02/24-23:32:42 WARN ReaderZooming onReadSettings
02/02/24-23:32:42 WARN ReaderView onReaderReady
02/02/24-23:32:42 WARN ReaderPaging onReaderReady end

ReaderZooming onReadSettings is called after ReaderPaging onReadSettings (That calls onPageUpdate).
Used here:
https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerview.lua#L698
and called here:
https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerview.lua#L966

So we use onReaderReady as the last resource, or is there a better way to control the events order? Should we put zoom onReadSettings first if it doesn't break anything?

[poire-z]

Should we put zoom onReadSettings first if it doesn't break anything?

That's always a bit risky (you don't know before you try that if Zooming won't need something from Paging :) so best to not take chances and don't break what's not broken).

But,if you need stuff from Zooming, and all that you want is really related to doing the right thing for some zoom/scroll modes, just have the complementary adjustments logically done in ReaderZooming:onReadSettings(), no ? Either by duplicating the little part of the work you need from onPageUpdate, or by calling onPageUpdate a second time when you need it (if there are conditions under which it's needed or not needed, it's fine if you use them here as some kind of documentation, ie

if not top_to_bottom then
  -- position us on the page as expected
end

[hugleo]

Should we put zoom onReadSettings first if it doesn't break anything?

That's always a bit risky (you don't know before you try that if Zooming won't need something from Paging :) so best to not take chances and don't break what's not broken).

But,if you need stuff from Zooming, and all that you want is really related to doing the right thing for some zoom/scroll modes, just have the complementary adjustments logically done in ReaderZooming:onReadSettings(), no ? Either by duplicating the little part of the work you need from onPageUpdate, or by calling onPageUpdate a second time when you need it (if there are conditions under which it's needed or not needed, it's fine if you use them here as some kind of documentation, ie

if not top_to_bottom then
  -- position us on the page as expected
end

I sure do hope that we don't need to use recalculate:

--[[
This method is supposed to be only used by ReaderPaging
--]]
function ReaderView:recalculate()

https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerview.lua#L675

that is called from ReaderView:onPageUpdate:
https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerview.lua#L965

It's supposed to be only used by ReaderPaging, and we would be using it for ReaderZooming.

More logs:

02/03/24-07:21:17 WARN recalculate() Entered in recalculate function
02/03/24-07:21:17 WARN recalculate() Entered in recalculate function
02/03/24-07:21:17 WARN recalculate() Entered in recalculate function
02/03/24-07:21:17 WARN recalculate() Entered in recalculate if self.ui.paging and self.state.page then
02/03/24-07:21:17 WARN recalculate() About to check if self.ui.zooming.zoom_bottom_to_top then
02/03/24-07:21:17 WARN ReaderView onPageUpdate end
02/03/24-07:21:17 WARN ReaderPaging onPageUpdate end
02/03/24-07:21:17 WARN recalculate() Entered in recalculate function
02/03/24-07:21:17 WARN recalculate() Entered in recalculate if self.ui.paging and self.state.page then
02/03/24-07:21:17 WARN recalculate() About to check if self.ui.zooming.zoom_bottom_to_top then
02/03/24-07:21:17 WARN ReaderZooming onPageUpdate end
02/03/24-07:21:17 WARN ReaderPaging onReadSettings end
02/03/24-07:21:17 WARN ReaderZooming onReadSettings
02/03/24-07:21:17 WARN ReaderView onReaderReady
02/03/24-07:21:17 WARN ReaderPaging onReaderReady end

Is checking things from zoom:
02/03/24-07:21:17 WARN recalculate() About to check if self.ui.zooming.zoom_bottom_to_top then

But zoom settings are read later:
02/03/24-07:21:17 WARN ReaderZooming onReadSettings

self.ui.zooming.zoom_bottom_to_top seems more like a page setting than a zoom setting :) But the values, when changed, are configured there in zoom module. Maybe a fallback check if the config is not yet read in the zoom module, we first read it in the page onReadSettings and use that for the first run.

[hugleo]

How about put self:setZoom() at the end of this function?

https://github.com/koreader/koreader/blob/master/frontend/apps/reader/modules/readerzooming.lua#L218

Or call onZoomUpdate event on function ReaderPaging:onReaderReady()

[poire-z]

I don't know, I can't say :) I'm not confortable with all this, and don't want to be and follow you on the adventure :)
Just pointing out how twisted and fragile all that is. And my only criteria for judging it is if reads nice and logical :)
If it's too complex and cloudy, just go with onReaderReady and do the additional stuff if not top_to_bottom or not left_to_right (whatever the conditions for needing that additional positioning to go out of the default anchor of top left of page at top left of screen).

[poire-z]

Ok, this doesn't look too bad :)
A bit sad to put here some logic (about configurable.zoom_direction) that should better be only in ReaderZooming (I guess you do that because ReaderZooming may not yet be available), but it looks like we already do the same kind of thing just above for configurable.writing_direction, so the sadness feels a bit consistent and we'll soon forget about it :)

[poire-z]

Getting a crash when opening a... .webp document (extract it from the zip, and open the .webp):
animated-webp-supported.zip
Same with this animated gif: 169px-Roman_Republic_Empire_map.zip

frontend/apps/reader/modules/readerview.lua:698: attempt to compare number with nil
stack traceback:
 frontend/apps/reader/modules/readerview.lua:966: in function 'handleEvent'
 frontend/ui/widget/container/widgetcontainer.lua:83: in function 'propagateEvent'

This is a line modified by this PR, so hope it is still fresh :)

[hugleo]

I would say: Just check for nil
@poire-z says: Would be good if you find the cause, you can learn more 10%

[hugleo]

Who cares? Pan zoom is only applicable for mupdf anyway.

[Frenzie]

And DjVu. ;-)