Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is it possible to enable 'Dual Pages' also in portrait format? #7129

Closed
grabi678 opened this issue Jan 10, 2021 · 43 comments · Fixed by #7138
Closed

Is it possible to enable 'Dual Pages' also in portrait format? #7129

grabi678 opened this issue Jan 10, 2021 · 43 comments · Fixed by #7138
Milestone

Comments

@grabi678
Copy link

This would be helpful for reading certain publications on e-ink readers with 8" or higher screen size.

@poire-z
Copy link
Contributor

poire-z commented Jan 10, 2021

Not easily - it's not just a matter of removing this enabled_func check:

{
name = "visible_pages",
name_text = _("Dual Pages"),
toggle = {_("off"), _("on")},
values = {1, 2},
default_value = 1,
args = {1, 2},
default_arg = 1,
event = "SetVisiblePages",
current_func = function()
-- If not in landscape mode, shows "1" as selected
if Device.screen:getScreenMode() ~= "landscape" then
return 1
end
-- if we return nil, ConfigDialog will pick the one from the
-- configurable as if we hadn't provided this 'current_func'
end,
enabled_func = function(configurable)
return Device.screen:getScreenMode() == "landscape" and
optionsutil.enableIfEquals(configurable, "view_mode", 0) -- "page"
end,

crengine (our EPUB engine) will itself prevent going dual page in some conditions:

function ReaderRolling:onSetVisiblePages(visible_pages)
-- crengine may decide to not ensure the value we request
-- (for example, in 2-pages mode, it may stop being ensured
-- when we increase the font size up to a point where a line
-- would contain less that 20 glyphs).
-- crengine may enforce visible_page=1 when:
-- - not in page mode but in scroll mode
-- - screen w/h < 6/5
-- - w < 20*em
-- We nevertheless update the setting (that will saved) with what
-- the user has requested - and not what crengine has enforced.
self.visible_pages = visible_pages
local prev_visible_pages = self.ui.document:getVisiblePageCount()
self.ui.document:setVisiblePageCount(visible_pages)

Would need to remove bits in this check:
https://github.com/koreader/crengine/blob/997a9a8fbe4486942997adf05c3516af32512b08/crengine/src/lvdocview.cpp#L3441-L3444
Dunno how this would affect CoolReader thus.

Would look like this:
image

Not really nice, but might be useful to people with bad progressive glasses with a very narrow reading view and a stuck neck :)

So, not currently possible, but might be. Waiting for more feedback if we should.

@grabi678
Copy link
Author

Yes, there are myopic readers who can easily read small fontsizes without glasses. Aside from that devices are getting bigger such as InkPad X . In my opinion this would give the reader a more fexible way to use his device. If it is not too much effort I would appreceate this option could be enabled also for portrait format.

@zwim
Copy link
Contributor

zwim commented Jan 11, 2021

Yes, I am severely nearsighted.
I did a quick test with the changes suggested by @poire-z.
On my Tolino4HD (6" 1448x1072) the result is not really usable. But on the Epos2 (8"1440x1920), when using a smaller font-size books are good readable with two columns in portrait. The only thing is, that the font-size has to be chosen so, that there are approximately 40 characters in a line.
I would suspect, that on larger devices the experience would be even better.

@poire-z
Copy link
Contributor

poire-z commented Jan 12, 2021

Ok, I can get that working. 2 questions:

  • We have one setting for this, so people enabling dual pages in landscape mode will get it enabled too when switching to portrait mode. Is that ok? Real use cases for reading in dual page in landscane, but when switching to portrait, expecting a single page? (I hope not, I don't want to complicate the handling :)
  • There's a thing in crengine: with a color screen, it draws a thin grey line as a separator between the 2 pages - but on a grey/eInk screen, it doesn't (can be checked on Android by toggle Screen> Color rendering. Would we prefer to have this coherent? And if yes: with or without this thin grey line? What do you prefer? (Hoping we'll have a consensus and it doesn't have to be a setting :)

(Although we have room on that left most bottom panel, so we could have 1 or 2 more toggles - but let's pretend I didn't wrote that :)

@zwim
Copy link
Contributor

zwim commented Jan 13, 2021

* We have one setting for this, so people enabling dual pages in landscape mode will get it enabled too when switching to portrait mode. Is that ok? Real use cases for reading in dual page in landscane, but when switching to portrait, expecting a single page? (I hope not, I don't want to complicate the handling :)

I don't think switching from landscape to portrait is a big problem, because the content will have to get rendered anyway. So there are different line and page breaks.

Maybe it would be a good idea not to close the bottom menu on rotation. Then the user can decide if he/she wants two pages/columns or one page/column on rotation.

* There's a thing in crengine: with a color screen, it draws a thin grey line as a separator between the 2 pages - but on a grey/eInk screen, it doesn't ...? What do you prefer? (Hoping we'll have a consensus and it doesn't have to be a setting :)

I have no real preference about the gray separator line.

@zwim
Copy link
Contributor

zwim commented Jan 13, 2021

Another idea: in portrait mode two pages are more two columns. Would it be possible to name the option something like "two columns" in portrait and "two pages" in landscape mode?

@poire-z
Copy link
Contributor

poire-z commented Jan 13, 2021

Maybe it would be a good idea not to close the bottom menu on rotation. Then the user can decide if he/she wants two pages/columns or one page/column on rotation.

Well, that's a bit more difficult than for all the other cases, as there's a change in geometry and everything is closed to be later recreated with the new size. So, not taking this painful idea :)

Another idea: in portrait mode two pages are more two columns. Would it be possible to name the option something like "two columns" in portrait and "two pages" in landscape mode?

May be from how it would look - but they are still 2 pages: a tap will increment the page count in the footer by 2, the Skim To +1/-1 buttons will do nothing every other tap. Solving this (that I don't intend to) would make them 2 columns in all cases.
So, rather not introduce a change in wording that doesn't really reflect in any real behaviour change.

@Frenzie
Copy link
Member

Frenzie commented Jan 13, 2021

Another idea: in portrait mode two pages are more two columns. Would it be possible to name the option something like "two columns" in portrait and "two pages" in landscape mode?

I think that's confusing more than anything, even if it's more accurate in some ways. :-)

@WS64
Copy link
Contributor

WS64 commented Jan 13, 2021

I only read in dual page and also would love to try it in portrait mode.
My opinion, only one dual page mode, valid for both landscape and portrait.

@poire-z
Copy link
Contributor

poire-z commented Jan 13, 2021

OK, #7138 should be in tomorrow's nightly.
Please play with it tomorrow, and let me know if there's an obvious usability issue before next release (possibly next weekend).
(I removed the thin line separator, so you'll stop having it on color screens.)

@zwim
Copy link
Contributor

zwim commented Jan 13, 2021 via email

@Frenzie Frenzie added this to the 2021.01 milestone Jan 13, 2021
@zwim
Copy link
Contributor

zwim commented Jan 14, 2021

@poire-z: The only thing which is slightly annoying with two columns/pages in portrait mode is, that progress bar in the top status bar only uses the right half.
Apart from that I like the new feature very much!

@NiLuJe
Copy link
Member

NiLuJe commented Jan 14, 2021

That's... fun. Did it work in landscape?

@zwim
Copy link
Contributor

zwim commented Jan 14, 2021 via email

@poire-z
Copy link
Contributor

poire-z commented Jan 14, 2021

Thanks for the feedback.

that progress bar in the top status bar only uses the right half.

I guess it's because there's a cut in the line, to not have a long line kill the feeling of 2 pages. With the separator I removed:
image

Having the progress bar span over the separator would look bad.
Now that we removed the separator, we could make it longer (actually, just like on a single page), but that may still kill the feeling of 2 pages (but not for you as you consider them 2 columns :)
(But ok, we have a footer that spans the 2 pages...)

The code involved is around https://github.com/koreader/crengine/blob/a90c0be089b08e0b0af64289fb0fdc8ba6d5ca6b/crengine/src/lvdocview.cpp#L1683-L1773

@Frenzie
Copy link
Member

Frenzie commented Jan 14, 2021

Fwiw (since I don't really use the feature), I've never cared for that separator in the middle.

@zwim
Copy link
Contributor

zwim commented Jan 15, 2021

With the following patch would allow to show the full header is shown in dual page/columns (on portrait and landscape).

diff --git a/crengine/src/lvdocview.cpp b/crengine/src/lvdocview.cpp
index 8db1d4e3..d60df169 100644
--- a/crengine/src/lvdocview.cpp
+++ b/crengine/src/lvdocview.cpp
@@ -1326,7 +1326,7 @@ void LVDocView::getPageHeaderRectangle(int pageIndex, lvRect & headerRc) {
 		int h = getPageHeaderHeight();
 		headerRc.bottom = headerRc.top + h;
 		headerRc.top += HEADER_MARGIN;
-		headerRc.left += HEADER_MARGIN;
+		headerRc.left = HEADER_MARGIN;    //xxxx
 		headerRc.right -= HEADER_MARGIN;
 	}
 }
@@ -1699,6 +1699,7 @@ void LVDocView::drawPageHeader(LVDrawBuf * drawbuf, const lvRect & headerRc,
     //lUInt32 cl2 = getBackgroundColor();
     //lUInt32 cl3 = 0xD0D0D0;
     //lUInt32 cl4 = 0xC0C0C0;
+    lUInt32 clxxx = 0xC0C0C0;
 	drawbuf->SetTextColor(cl1);
 	//lUInt32 pal[4];
 	int percent = getPosPercent();
@@ -1952,9 +1953,10 @@ void LVDocView::drawPageTo(LVDrawBuf * drawbuf, LVRendPageInfo & page,
 			if (page.index & 1) {
 				// right
 				phi &= ~PGHDR_AUTHOR;
+				phi &= ~PGHDR_TITLE;
             } else {
 				// left
-				phi &= ~PGHDR_TITLE;
+//				phi &= ~PGHDR_TITLE;
                 phi &= ~PGHDR_PERCENT;
                 phi &= ~PGHDR_PAGE_NUMBER;
 				phi &= ~PGHDR_PAGE_COUNT;

@poire-z
Copy link
Contributor

poire-z commented Jan 15, 2021

^ the progress bar is now wide, but I don't see the book title, just the author:

image

Another idea if we don't want to disrupt how it shows currently is just to have the progress bar being 2 segments, so it would just skip the blank.

Pinging CoolReader's @virxkane - as this is the main status bar in CoolReader, to be involved in the discussion if we're going to change it (only how it look in "2 pages modes / VisiblePages == 2")

@zwim
Copy link
Contributor

zwim commented Jan 15, 2021

Interresting: One book shows Author+Title
grafik
but other books show just the Author
grafik

@virxkane
Copy link

Pinging CoolReader's @virxkane - as this is the main status bar in CoolReader, to be involved in the discussion if we're going to change it (only how it look in "2 pages modes / VisiblePages == 2")

Sorry, but I have accumulated a lot of unfinished business, and this code is not familiar to me, so I cannot help right now. Later, I or someone else will have to deal with this :(

@virxkane
Copy link

Interresting: One book shows Author+Title

It seems there is a limitation on the length, in the usual one-page mode, also, if it does not fit, then the author and the title are displayed alternately. I.e on one page - the author, on the next - the title.

@poire-z
Copy link
Contributor

poire-z commented Jan 15, 2021

Sorry, but I have accumulated a lot of unfinished business, and this code is not familiar to me, so I cannot help right now. Later, I or someone else will have to deal with this :(

That's alright! :)
I was not so much requesting your help on the code - than your opinion on the top status bar in the various CoolReader frontends.
When in 2-pages mode (which might be rare, dunno, may be not rare in the QT versions used on PC screen), would you (and your users) be fine with it being a single bar taking the full width (so, no break in the middle) - or does the fact that 2-pages mean 2 top status bar (with each different infos) feels like some UI thing of value?
If it doesn't, we can just make it the same wide top status bar in both 1-page and 2-pages mode.
It if does, well, we'll think again :)

@zwim
Copy link
Contributor

zwim commented Jan 15, 2021

Interresting: One book shows Author+Title

It seems there is a limitation on the length, in the usual one-page mode, also, if it does not fit, then the author and the title are displayed alternately. I.e on one page - the author, on the next - the title.

@virxkane: Thanks, that was the problem.

The following diff is not quite beautiful (empty statement), but does the trick to show what it will look like :)

diff --git a/crengine/src/lvdocview.cpp b/crengine/src/lvdocview.cpp
index 8db1d4e3..c1891ac8 100644
--- a/crengine/src/lvdocview.cpp
+++ b/crengine/src/lvdocview.cpp
@@ -1326,7 +1326,7 @@ void LVDocView::getPageHeaderRectangle(int pageIndex, lvRect & headerRc) {
 		int h = getPageHeaderHeight();
 		headerRc.bottom = headerRc.top + h;
 		headerRc.top += HEADER_MARGIN;
-		headerRc.left += HEADER_MARGIN;
+		headerRc.left = HEADER_MARGIN;    //xxxx
 		headerRc.right -= HEADER_MARGIN;
 	}
 }
@@ -1717,12 +1717,13 @@ void LVDocView::drawPageHeader(LVDrawBuf * drawbuf, const lvRect & headerRc,
 //		cl4 = cl1;
 //		//pal[0] = cl1;
 //	}
+/* xxxx Maybe some other dependencies on left-page can be cleaned, too.
 	if ( leftPage )
 		drawbuf->FillRect(info.left, gpos - 2, info.right, gpos - 2 + 1, cl1);
         //drawbuf->FillRect(info.left+percent_pos, gpos-gh, info.right, gpos-gh+1, cl1 ); //cl3
         //      drawbuf->FillRect(info.left + percent_pos, gpos - 2, info.right, gpos - 2
         //                      + 1, cl1); // cl3
-
+*/
 	int sbound_index = 0;
 	bool enableMarks = !leftPage && (phi & PGHDR_CHAPTER_MARKS) && sbounds.length()<info.width()/5;
 	int w = GetWidth();
@@ -1893,7 +1894,7 @@ void LVDocView::drawPageHeader(LVDrawBuf * drawbuf, const lvRect & headerRc,
 		}
 		int w = info.width() - 10;
 		if (authorsw + titlew + 10 > w) {
-			if ((pageIndex & 1))
+			if ((pageIndex & getVisiblePageCount()))
 				text = title;
 			else {
 				text = authors;
@@ -1951,9 +1952,11 @@ void LVDocView::drawPageTo(LVDrawBuf * drawbuf, LVRendPageInfo & page,
 		if (getVisiblePageCount() == 2) {
 			if (page.index & 1) {
 				// right
-				phi &= ~PGHDR_AUTHOR;
+//				phi &= ~PGHDR_AUTHOR;
+        ; /* empty statement!!!! */
             } else {
 				// left
+				phi &= ~PGHDR_AUTHOR;
 				phi &= ~PGHDR_TITLE;
                 phi &= ~PGHDR_PERCENT;
                 phi &= ~PGHDR_PAGE_NUMBER;

@virxkane
Copy link

than your opinion on the top status bar in the various CoolReader frontends.

I don't use 2-page mode, so my opinion is this: it should be the most obvious and intuitive.
Or what is most simple: leave it as it is, i.e. what the 2-page mode looks like now.
Clipboard08
In landscape mode, it should probably look as much like a paper book as possible because that's the first association that comes up, i.e. 2 status bars.
book_zzz

But in portrait mode, when displaying 2 pages, it seems intuitively that there should be one status bar. In addition, in this mode, the 2nd status bar may simply not fit.

@virxkane
Copy link

I think it makes sense to also ask @hius07's opinion.

@poire-z
Copy link
Contributor

poire-z commented Jan 15, 2021

^^ makes sense.
CoolReader currently can't get 2-pages in portrait mode (needs an additional param to setVisiblePageCount()), so we won't sadden anybody by going at it differently :)
@zwim : you could go at tweaking it by just having early:
bool isPortraitMode = fullRect.bottom > fullRect.right;
and applying tweaks a bit more cleanly if (we have two pages && isPortraitMode)

Or may be cleaner: just not take the 2 pages paths when isPortraitMode.

(An additional improvement to landscape mode would be to have the progress bar displays ticks and progress on the 2 segments.)

@zwim
Copy link
Contributor

zwim commented Jan 15, 2021

I have created PR koreader/crengine#408. So we can discuss easier.

@poire-z
Copy link
Contributor

poire-z commented Jan 20, 2021

Some discussion has continued in koreader/crengine#408 - but getting back on it here as it's more visible than there.

If all that happens to be working well, are we all fine to go with that too in landscape mode, for consistency? With twoVisiblePagesAsOnePageNumber=true, meaning: single wide header, current page number and total page count being the number of screen views - so what we see would need to be thought of as 2-columns on a single page, and no longer 2-pages. (We then would need to rename "Dual Page" to "Two Columns").
I'd prefer this to be hardcoded and not a user toggle, it's a KOReader choice to present things that way (CoolReader would get the current behaviour, until they decide to set the new property and may be need some frontend tweaks) - and it's the same in landscape and portrait, no reason (except old habits if really hard to lose) to change the meaning/page-number-steps in the footer whether in portrait vs landscape.
(If not, and we'd still want to keep dual-page and make it a first class citizen, a few other tweaks would be needed, like in the SkimTo widget, replacing +1 with +2 - so we don't get half the taps ineffective :)

A few more questions / requests for opinion related to all this.

A This issue would not exist in "2-column-as-1-page" mode, but it exists in the current "2-pages" mode: when showing 2 pages, which page number should the footer and header show: the page number of the left page or the one of the right page ? Because currently, header and footer disagree:
image
On this, when showing the first and second page of a book, which unique page number feels the right one to show?

B This also happens in one-page mode: when one (or two) pages shows more than one chapter, which chapter to use to display in the footer ? The first one, or the last one in view?
Currently in 2-pages mode:
image
which would be in 2-column-as-1-page:
image
I would personally expect all to show as current chapter: "4.5 Property".
Thoughts?

C With "2-column-as-1-page", which is just masquerading page numbers while still having crengine working with them as 2-pages, how far should we push the "2-column-as-1-page" principle with page-breaks ?
This publisher page break would be a column break:
image
Should we add a blank "column" on the right so it really becomes a page/view break?
Or is it just ok to consider page-breaks as column-breaks ?

@Frenzie
Copy link
Member

Frenzie commented Jan 20, 2021 via email

@hius07
Copy link
Member

hius07 commented Jan 20, 2021

I'd like a publisher page break to be a page break, so to add a blank column.

@zwim
Copy link
Contributor

zwim commented Jan 20, 2021

To question A: I would go for the number of the right page (as the pagenumber is on the right half of the view).
To question B: The last one (The books have checked do it that way).
To C: I would go page break is just a column break. A new chapter may start on the left side/column, but please not on every pagebrak.

@poire-z
Copy link
Contributor

poire-z commented Jan 20, 2021

To question A: I would go for the number of the right page (as the pagenumber is on the right half of the view).

It's not only used there: it could be in the footer where page number could appear on the left side, it's what appears in the middle in the SkimTo widget, it's what is part of the bookmark when bookmarking or highlighting... So, I don't think we should think about it only from a visual position point of view.

To question B: The last one (The books have checked do it that way).

What do you mean with The books have checked do it that way ? That you did check some paper book ? But a paper book doesn't tell you what chapter is displayed when you have it opened and are reading 2 pages :/ (?)

I'd like a publisher page break to be a page break, so to add a blank column.
To C: I would go page break is just a column break.

Will need more opinions on that one :) It's quite a lot more cumbersome to add the blank column (as the setting would have to be forwarded and handled lower in the rendering code - without that, I don't have to, it's just upper in the page number interface)

A new chapter may start on the left side/column, but please not on every pagebrak.

You mean you're fine with a new chapter may start on the right column ? (Because with page break = page break/blank column means all chapters will start on the left column.)
(I think the question is not about adding a blank left page to make it look like in paper book where new chapters start on the right page - but the contrary, whether to add a blank right page or not so chapters start on the left page, which makes it really unlike paper books :/ so may be that's another question :) :/)

@NiLuJe
Copy link
Member

NiLuJe commented Jan 20, 2021

Not the target audience (like, at all), but, FWIW:

A. First page.
B. As you mentioned, first heading.
C. Column break.

@Frenzie
Copy link
Member

Frenzie commented Jan 20, 2021

Not the target audience (like, at all)

I'm a possibly somewhat unique form of target audience, namely the large window on desktop type. I also want that in paged media, like Zathura (and just about any other desktop reader, but Zathura feels more similar than Evince or Adobe Reader). But a very mild form of want, or I'd have added it years ago. ;-)

@zwim
Copy link
Contributor

zwim commented Jan 20, 2021

It's not only used there: it could be in the footer where page number could appear on the left side, it's what appears in the middle in the SkimTo widget, it's what is part of the bookmark when bookmarking or highlighting... So, I don't think we should think about it only from a visual position point of view.

OK, then the first page (or maybe both 3-4/120).

What do you mean with The books have checked do it that way ? That you did check some paper book ? But a paper book doesn't tell you what chapter is displayed when you have it opened and are reading 2 pages :/ (?)

Some of my printed books are really clever, they know on which chapter you read:
grafik

To C: I would go page break is just a column break.

I meant pagebreak should start a new column.

@Frenzie
Copy link
Member

Frenzie commented Jan 20, 2021

Some of my printed books are really clever

= use LaTeX xD

@NiLuJe
Copy link
Member

NiLuJe commented Jan 20, 2021

OK, then the first page (or maybe both 3-4/120).

That's an interesting thought ;).

I meant pagebreak should start a new column.

i.e., the behavior in @poire-z's original screenshot, right?

@hius07
Copy link
Member

hius07 commented Jan 20, 2021

It's quite a lot more cumbersome to add the blank column

No problem, let it be a column break.

@poire-z
Copy link
Contributor

poire-z commented Jan 20, 2021

To question B: The last one (The books have checked do it that way).
Some of my printed books are really clever, they know on which chapter you read:

:) Nice book, which seems to answer the opposite of what you wrote, doesn't it?: on the right page, it shows the first chapter in this right page (3.10.2...), not the last chapter in this right page (3.10.3...).

I meant pagebreak should start a new column.

I got that. It's the next part of your answer that I didn't get (A new chapter may start on the left side/column, but please not on every pagebrak.)

@hius07
Copy link
Member

hius07 commented Jan 20, 2021

So, what are the advantages of Dual Pages over Two Columns in landscape?
Two alt bars and double page counting? That's all? Is it worth the loss of unification?

@poire-z
Copy link
Contributor

poire-z commented Jan 20, 2021

what are the advantages of Dual Pages over Two Columns in landscape?
Two alt bars and double page counting? That's all? Is it worth the loss of unification?

I'm with you at wanting them unified :) mostly for the unification and avoiding the complexities of handling the 2 cases (and fixing the Dual Pages bugs I've seen that would just not be there with Two Columns).
It's just that in landscape, it seems to me it's less natural/intuitive to think of them as 2 columns.
(But I don't use landscape, so, the more users tell me it's fine, the sooner I'll forget that thought :)

@zwim
Copy link
Contributor

zwim commented Jan 20, 2021

To question B: The last one (The books have checked do it that way).
Some of my printed books are really clever, they know on which chapter you read:

:) Nice book, which seems to answer the opposite of what you wrote, doesn't it?: on the right page, it shows the first chapter in this right page (3.10.2...), not the last chapter in this right page (3.10.3...).

Yes, the first on the actual (=right) page (but not the first on the actual double page).

It's the next part of your answer that I didn't get (A new chapter may start on the left side/column, but please not on every pagebrak.)

I meant: If a new chapter (in LaTex = h1 in html) starts, it would look good, if it would start on the left column/page. But that is not a strong vote.
The other section/subsection... (=h2, h3, ...) should not start a new column per se. Only if there is a pagebreak before, they shall start on a new column.

@poire-z
Copy link
Contributor

poire-z commented Jan 22, 2021

Also pinging @WS64 as a dual-page user, about the above questions and discussions.

Some other questions to dual page users - can't test that at the moment: how do Reading statistics do with the current dual page, that skips one page number out of two? Do you end up with Page reads = half of the total number of pages?
(If that's the case, it might be the definitive argument to drop two-pages, as 2-column on a single page would just fit better with how we count pages).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants