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

cre landscape view: adds 1 page / 2 pages toggable #4782

Merged
merged 7 commits into from Mar 13, 2019

Conversation

Projects
None yet
3 participants
@poire-z
Copy link
Contributor

commented Mar 13, 2019

Fix a few small issues, and introduce a new toggle in the Orientation bottom config menu:

2pagesb

Note: 2-pages view was the default for landscape for some users depending on their device resolution and selected dpi, and was not toggable. So I guess a few people will discover this existing nice feature of crengine, that I find really cute.

See individual commit messages for details.

Added fixes for CoverBrowser issues detailed in #4778. Closes #4778.

Also:
bump crengine koreader/crengine#270

  • docToWindowPoint(): adds fitToPage parameter
  • 2-pages mode: add and fix some functions
  • 2-pages mode: tweak middle margin sizing
  • Text rendering: fix words stuck when hyphenating on italic

bump base koreader/koreader-base#850

  • cre.cpp: add a few functions useful with 2-pages mode
  • cre.cpp: fix docToWindowRect() to get truncated segments for lines split onto 2 pages, for highlights and links
  • Bump CMake minimum version to 3.5.1 koreader/koreader-base#849

@poire-z poire-z changed the title Landscape 2pages cre landscape view: adds 1 page / 2 pages toggable Mar 13, 2019

@poire-z poire-z force-pushed the poire-z:landscape_2pages branch from e6a0dc4 to 5f5a9eb Mar 13, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Right, I never got around to finishing #4296. I could update the old Docker image to include a newer CMake, but if you're okay having this wait for a couple of days I'll just properly finish that one.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

2 notes:

  • if people are going to play with orientation and margins like I did in #4752 (comment) - they may get cpuloops and need hard reboot and lose their settings - so we may want to set jit.opt.start("loopunroll=45") on Kobo quick :) Pinging @NiLuJe

  • There is something with using buttonprogress for margins (#4702), that often causes a double painting and refreshing of the page (1st paint after margin changed, 2nd paint when repositionning to previous xpointer, so possible on another page).
    I still have to fix that, but it's a bit twisted, as this buttonprogress is used for contrast and do things early unlike other bottom widgets - and when I began fixing that on cre side, none were working with pdf :) so...

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Ok. But you could have waited for merging koreader/koreader-base#850 as I wrote no hurry. I hurried finishing the frontend side this morning so it could be bumped and you could go on with your CMake experimentations :|

But ok, no hurry, this can wait.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@poire-z I didn't mean for you to hurry, my apologies. You can now rebase on top of master and it should work. :-)

poire-z added some commits Mar 13, 2019

bump crengine: 2-pages view tweaks, fix hyphenating on italic
Includes:
- docToWindowPoint(): adds fitToPage parameter
- 2-pages mode: add and fix some functions
- 2-pages mode: tweak middle margin sizing
- Text rendering: fix words stuck when hyphenating on italic

cre.cpp:
- add a few functions useful with 2-pages mode
- fix docToWindowRect() to get truncated segments for
  lines split onto 2 pages, for highlights and links
cre: Footnotes popup: fix markers disappearing too early
When following a footnote popup, the highlighted link
delayed clearing (when the link is hidden by the popup)
would clear the margin marker on the target page too early.
cre: tweak current page highlights detection
No need to use current page and xpointers. We can use
'pos', in both scroll and page modes, as it is always
accurate to show the y of the current view.

@poire-z poire-z force-pushed the poire-z:landscape_2pages branch from 5f5a9eb to f4a00d5 Mar 13, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Thanks, that couple of days went fast :)
(I just dont like these interleaving bumps - been painful in the past - and as I don't get any of your CMake stuff, I thought you would possibly need more iterations on both frontend and base and I didn't want to be in the middle of them :)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

So, back to language business: Visible pages: one | two or Something else: one page | two pages ?

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

This one is just a thought I had last night based on an off-hand remark by @NiLuJe on Gitter yesterday, so you could call it experimental. (But I tried the build on my PC, my H2O and the Android emulator, so I think it's fine.) The very module I added in that PR was actually added to CMake 3.11 itself as FetchContent.

But merely bumping the minimum version requirement was fully prepared around the time I opened that other PR (i.e., October 2018). I just had to make/push a new Docker image with both Python and SDL for CI purposes, but the nightly builds have been running on Ubuntu 16.04 since October. And of course Travis in base was lagging behind as well.

Like I said in koreader/koreader-base#849, CMake 3.3 "is slightly less annoyingly stupid about finding libraries," implicitly meaning there's basically no immediate effect other than being able to remove a few workarounds related to that kind of thing. ;-)

So, back to language business: Visible pages: one | two or Something else: one page | two pages ?

How about simply Two pages: on/off?

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@poire-z : I was giving it a few days to confirm that it worked around the issue in your case ;). I'll take care of it :).

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Why to switch to Meson ;-)

I see, more readable. Well, thank you both for taking care of all that ugly build side :)

How about simply Two pages: on/off?

Could work, but inverted to avoid some kind of ugly X (crossed checked toggles) in portrait mode:

image

image

But I dunno, in the same way we avoid having Yes|No in ConfirmBox, I like to have named buttons, when we can (althought off|on makes it easier for translators :)

@poire-z poire-z force-pushed the poire-z:landscape_2pages branch from f4a00d5 to 6b324e9 Mar 13, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

OK, let's go with Two pages: off | on.
Added fixes for CoverBrowser issues detailed in #4778. Closes #4778.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I'd be partial to something like Split view or Two columns, but that's just me ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Or, perhaps more adapted to the specific context: Double spread?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Two Columns would be clear for most I guess, but "double spread" doesn't talk to me :) and "Split view" does not announce if it's vertical or horizontal :)

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Yeah, there's usually a Left|Right following Split View (or a Vertical preceding it) ;).

poire-z added some commits Mar 13, 2019

cre landscape view: adds 1 page / 2 pages toggable
Adds a toggle switch in the Orientation bottom config menu to
allow showing 1 page or 2 pages when in landscape mode.
Previously, this was hardcoded to be in 2-pages modes only
in some circumstances (device resolution + user dpi).
cre 2-pages view: fix markers when following links and back
Show markers in the middle margin when target is in the right page.
cre 2-pages view: allow extending selection across pages
Similar to what's been added for 1 page view, but just turn
one page instead of switching to scroll mode when reaching
top left or bottom right corners.
Also make the selection start xpointer more accurate by
getting them in onHold(), instead of possibly too late in
onHoldPan() where we have already moved.
CoverBrowser: fix a few "Extract and cache" issues
Ignore path and cover specs when last invoked menu
was History.
Fix InfoMessage size, broken by InfoMessage auto resize.

@poire-z poire-z force-pushed the poire-z:landscape_2pages branch from 6b324e9 to c483464 Mar 13, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Somehow "Two Columns" is common and I quite like it. But "Two Pages" (fixed capitalization) says more what it is: page numbers will skip by 2 when taping or swiping, and the middle margin, when large, feels more like a book than real 2 columns in scientific papers or newspapers.

@NiLuJe

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Stupid question: Would bumping base without having this bit in break something?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

I guess no. But let me not have us wonder about that :)

@poire-z poire-z merged commit ad7dc86 into koreader:master Mar 13, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:landscape_2pages branch Mar 13, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I wouldn't call it a double spread. I can quickly pull out an example of one if you like. :-P

You could call it dual pages.

But I dunno, in the same way we avoid having Yes|No in ConfirmBox, I like to have named buttons, when we can (althought off|on makes it easier for translators :)

True, but it's a contextual thing. On/off here is a simple checkbox without any potential for ambiguity.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Here, I'd say something like this is a proper dual spread. ;-)

2019-03-13 22 34 21

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

You could call it Dual Pages

I kinda like that. It's less abrupt than "Two Pages", sounds more like a feature than can be enabled/disable with some off|on - possibly a bit cryptic but would make sense once seen.

@Frenzie Frenzie added this to the 2019.04 milestone Mar 13, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@poire-z If you'd like to go with that, please push it to master before tomorrow's push to Transifex. ;-)

I prefer dual pages too but I don't think there's anything wrong with two pages.

I don't have Word at hand atm, but Writer calls it "book view" for the style discussed here and "multiple-page view" for whichever number will fit.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

OK, adding it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.