Option to change page gap in continuous mode

[mustafa-001]

for request see #5656

Adds an option to select page gap in continuous mode. Option only shows up on documents that have numbered pages (.pdf, .cbz) because i think there is not much use to this option when reading arbitrarily paged documents. If seen necessary same logic can be duplicated for .epub files.

koptoptions.lua handles user selection and saving that selection
ReaderView:readSettings() handles reading previously selected option when opening book
ReaderVİew:onPageGapUpdate() renders changes when selection made

Page gap is same in both landscape and portrait modes, I think adding different settings for both is little unnecessary.
The options for gap is 0, 8, 16 and 32, default one before was 8.

commit log

page_gap setting on opening a book
if it does not exist defaults to 8
-adds page_gap selector to koptoptions.lua,
user can select 0, 8, 16, 32 via footer settings
-adds :onPageGapUpdate event handler to ReaderView

---

This change is 

[Frenzie]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @mustafa-001)

---

frontend/apps/reader/modules/readerview.lua, line 850 at r1 (raw file):

end

function ReaderView:onPageGapUpdate(page_gap)

Shouldn't there be some kind of event emitted after changing these page gap variables?

[mustafa-001]

koreader/frontend/ui/data/koptoptions.lua

Lines 8 to 11 in bfe9cd8

	local KoptOptions = {
	prefix = 'kopt',
	needs_redraw_on_change = true,
	{

Changes on this settings forces screen wide refresh, and ReaderView:painttto() draws everything again. Also I didn't quite figure out what to do other than full screen refresh thats already queued :). Any suggestions on proper way of doing this are welcomed.

[NiLuJe]

Yeah, IIRC, kopt settings can force repaints without any specific event because... reasons? :D.

[Frenzie]

It probably mostly makes sense to just redraw everything after changing a setting… a bit wasteful depending on what it is perhaps but I doubt it's bothersome.

So if that's not an issue, it seems fine to me.

[mustafa-001]

Aww, when trying to separate landscape and portrait page gap I just realized they are both calculated from ReaderView.page_gap.height, page_gap.width works in horizontal scrolling (left-right, which I think not much useful and currently not implemented. Maybe with few and very long lines?).

So I'm removing width, moving height to :onreadSettings(), leaving page_gap.color only.

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

Lines 52 to 60 in 7fb2db8

	-- properties of the gap drawn between each page in scroll mode:
	page_gap = {
	-- width in pixels (when scrolling horizontally)
	width = Screen:scaleBySize(G_reader_settings:readSetting("page_gap_width") or 8),
	-- height in pixels (when scrolling vertically)
	height = Screen:scaleBySize(G_reader_settings:readSetting("page_gap_height") or 8),
	-- color (0 = white, 8 = gray, 15 = black)
	color = Blitbuffer.gray((G_reader_settings:readSetting("page_gap_color") or 8)/15),
	},

[yparitcher]

if you are getting rid of this you should also clean up

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

Lines 392 to 396 in 7fb2db8

	if self.scroll_mode == "vertical" then
	bb:paintRect(x, y, self.dimen.w, self.page_gap.height, self.page_gap.color)
	elseif self.scroll_mode == "horizontal" then
	bb:paintRect(x, y, self.page_gap.width, self.dimen.h, self.page_gap.color)
	end

and

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

Line 51 in 7fb2db8

scroll_mode = "vertical",

does anyone know what scroll_mode is used for, and if it is ever set to horizontal?

[Frenzie]

I renamed scroll mode to continuous in the GUI.

[yparitcher]

@Frenzie
scroll_mode or page_scroll?

[mustafa-001]

page_scroll is a boolean that holds True in continuous mode, False in page mode. scroll_mode set vertical in ReaderView creation and doesn't change.

[Frenzie]

That's part of the same thing, perhaps separated specifically for that vertical vs horizontal thing. I don't know the specific plan behind 63e5e7e

[yparitcher]

lgtm besides for the typo

[poire-z]

Yeah, IIRC, kopt settings can force repaints without any specific event because... reasons? :D.

koreader/frontend/ui/widget/configdialog.lua

Lines 909 to 913 in c3a0bd4

	if self.config_options.needs_redraw_on_change then
	-- Some Kopt document event handlers just save their setting,
	-- and need a full repaint for kopt to load these settings,
	-- notice the change, and redraw the document
	UIManager:setDirty("all", "partial")

Part of #4793, with some long discussion trying to understand how that works, that I'd rather not re-read :)
I guess that some options are only used by the kopt code, and there's no (or no need for) Lua code to handle some event, as there's no real action - except having kopt do its stuff with the updated settings.
For this one (page_gap.height), that's our Lua code that draws the gray line, so it looks allright to have an event and an handler (or we could just re-read the setting each time we draw a page, but no need to bother changing what's here I guess).

[Frenzie]

---

frontend/ui/data/koptoptions.lua, line 88 at r6 (raw file):

            {
                name = "page_gap_height",
                name_text = S.PAGE_GAP,

I'd just use the string here instead of adding it to that strings.lua file. For some reason those strings were globals back in 2013 but separating them from their surroundings just makes translation harder.

[Frenzie]

Reviewed 1 of 3 files at r1, 1 of 2 files at r3.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @mustafa-001)

[poire-z]

Is this ready to merge?
(I think @Frenzie last comment about S.PAGE_GAP should be adressed in a dedicated "chore" PR, where we'd do all of them in one batch.)

[Frenzie]

If you have no further comments, sure. I haven't tested it.

[poire-z]

I haven't tested it.

Neither did I, but that's what users are for :)

I guess the Circle CI build (repeatedly) fails because of the transiflex>weblate move, and this PR base commit is not adapted to the CircleCI updated tests?

[Frenzie]

Sort of, a minor accident on my part caused the submodule reference to become invalid.

My plan was simple, to update Weblate with the latest from Transifex when the time came, the end.

I did that, but it turned out Weblate had already attracted a few dozen new translations. So instead of overwriting I had to redo it by merging the two translations, and I semi-accidentally rebased that one commit. I wasn't fully cognizant of the fact that it'd change the commit hash if I just kept it in there.

tl;dr Now older commits referencing the submodule as it was can't find their reference.