Option to change page gap in continuous mode by mustafa-001 · Pull Request #5705 · koreader/koreader

mustafa-001 · 2019-12-24T01:58:56Z

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

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

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?

-explanation on long tap

mustafa-001 · 2019-12-24T14:35:12Z

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 · 2019-12-24T16:14:55Z

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

Frenzie · 2019-12-24T19:02:35Z

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 · 2019-12-26T17:24:57Z

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), 
        
           },

-page_gap.height defined in :onReadSettings -use Screen:scaleBySize() instead of direct number

yparitcher · 2019-12-26T19:26:00Z

    -- 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),


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?

I renamed scroll mode to continuous in the GUI.

@Frenzie
scroll_mode or page_scroll?

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.

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

-read kopt_page_gap_height from settings.lua if not exists in docsettings

yparitcher · 2019-12-27T03:28:22Z

lgtm besides for the typo

poire-z · 2019-12-27T22:59:39Z

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 · 2020-01-01T14:51:39Z

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 · 2020-01-02T19:41:51Z

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 · 2020-01-02T19:44:02Z

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

poire-z · 2020-01-02T19:55:24Z

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 · 2020-01-02T20:04:28Z

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.

Fixes koreader#5656.

The input value should go through Screen:scaleBySize, as in onReadSettings Otherwise, choosing an option from ConfigDialog leads to different results than on document open, as one is scaled, but not the other. Issue extant since the inception of page gap configration, way back in koreader#5705 (The intended and correct behavior if of course to scale this, e.g., the setting is a "koreader flex pixel", not an absolute actual px value ;).

The input value should go through Screen:scaleBySize(), as in onReadSettings Otherwise, choosing an option from ConfigDialog leads to different results than on document open, as one is scaled, but not the other. Issue extant since the inception of page gap configuration, way back in koreader#5705 (The intended and correct behavior if of course to scale this, e.g., the setting is a "koreader flex pixel", not an absolute actual px value ;).

The input value should go through Screen:scaleBySize(), as in onReadSettings Otherwise, choosing an option from ConfigDialog leads to different results than on document open, as one is scaled, but not the other. Issue extant since the inception of page gap configuration, way back in #5705 (The intended and correct behavior if of course to scale this, e.g., the setting is a "koreader flex pixel", not an absolute actual px value ;).

Fixes koreader#5656.

The input value should go through Screen:scaleBySize(), as in onReadSettings Otherwise, choosing an option from ConfigDialog leads to different results than on document open, as one is scaled, but not the other. Issue extant since the inception of page gap configuration, way back in koreader#5705 (The intended and correct behavior if of course to scale this, e.g., the setting is a "koreader flex pixel", not an absolute actual px value ;).

-ReaderView:onReadSettings reads documents

0c10b29

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

mustafa-001 changed the title ~~-ReaderView:onReadSettings reads documents~~ Option to change page gap in continous mode Dec 24, 2019

mustafa-001 changed the title ~~Option to change page gap in continous mode~~ Option to change page gap in continuous mode Dec 24, 2019

Frenzie reviewed Dec 24, 2019

View reviewed changes

-enable page gap option only in continuous mode

bfe9cd8

-explanation on long tap

yparitcher reviewed Dec 24, 2019

View reviewed changes

Comment thread frontend/apps/reader/modules/readerview.lua Outdated

-remove page_gap.width setting

9243f7b

-page_gap.height defined in :onReadSettings -use Screen:scaleBySize() instead of direct number

yparitcher reviewed Dec 26, 2019

View reviewed changes

Comment thread frontend/apps/reader/modules/readerview.lua Outdated

yparitcher reviewed Dec 26, 2019

View reviewed changes

mustafa-001 added 2 commits December 27, 2019 01:32

-remove references to page_gap.height

26a238a

-read kopt_page_gap_height from settings.lua if not exists in docsettings

-remove trailing whitespace

b853d2c

yparitcher reviewed Dec 27, 2019

View reviewed changes

Comment thread frontend/apps/reader/modules/readerview.lua Outdated

fix the typo on G_reader_settings:readSetting()

91ccd5d

yparitcher approved these changes Dec 27, 2019

View reviewed changes

poire-z reviewed Dec 27, 2019

View reviewed changes

Comment thread frontend/apps/reader/modules/readerview.lua Outdated

Frenzie added this to the 2020.01 milestone Dec 30, 2019

--indentation

455f1cd

Frenzie reviewed Jan 1, 2020

View reviewed changes

Frenzie added the enhancement label Jan 2, 2020

Frenzie merged commit 12adb96 into koreader:master Jan 2, 2020

mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020

[feat] Option to change page gap in continuous mode (koreader#5705)

a2e810c

Fixes koreader#5656.

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026

[feat] Option to change page gap in continuous mode (koreader#5705)

2f52a59

Fixes koreader#5656.

	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

Uh oh!

Uh oh!

Conversation

mustafa-001 commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

mustafa-001 commented Dec 24, 2019

Uh oh!

NiLuJe commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Dec 24, 2019

Uh oh!

Uh oh!

mustafa-001 commented Dec 26, 2019

Uh oh!

Uh oh!

yparitcher Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie Dec 26, 2019

Choose a reason for hiding this comment

Uh oh!

yparitcher Dec 26, 2019

Choose a reason for hiding this comment

Uh oh!

mustafa-001 Dec 26, 2019

Choose a reason for hiding this comment

Uh oh!

Frenzie Dec 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yparitcher commented Dec 27, 2019

Uh oh!

poire-z commented Dec 27, 2019

Uh oh!

Uh oh!

Frenzie commented Jan 1, 2020

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

poire-z commented Jan 2, 2020

Uh oh!

Frenzie commented Jan 2, 2020

Uh oh!

poire-z commented Jan 2, 2020

Uh oh!

Frenzie commented Jan 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mustafa-001 commented Dec 24, 2019 •

edited

Loading

NiLuJe commented Dec 24, 2019 •

edited

Loading

yparitcher Dec 26, 2019 •

edited

Loading