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: fix position and page number after window resize #4754

Merged
merged 2 commits into from Mar 10, 2019

Conversation

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

poire-z commented Mar 7, 2019

Just copied the code from the next function ReaderRolling:onChangeScreenMode into ReaderRolling:onSetDimensions with some conditionals to not have it before init() is done.
Seems to fix position, footer, toc...
Not super sure about the whole events chain and order, but it seems to work without issue, even on my Kobo when just rotating.
Pinging @NiLuJe - might be there's just a few lines to add around there to fix #4752 (comment)

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 8, 2019

Does anything ever fire a SetDimensions Event?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2019

Mostly SDL on window resize, and your forma-aware ReaderView:onSetScreenMode (and in an unused onSetFullScreen)

frontend/apps/reader/modules/readercropping.lua:    self.ui:handleEvent(Event:new("SetDimensions",
frontend/apps/reader/modules/readerview.lua:        self.ui:handleEvent(Event:new("SetDimensions", new_screen_size))
frontend/apps/reader/modules/readerview.lua:    self.ui:handleEvent(Event:new("SetDimensions", Screen:getSize()))
frontend/device/sdl/device.lua:                    UIManager:broadcastEvent(Event:new("SetDimensions", new_size))
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2019

Isn't setFullScreen related to skim mode or something?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2019

No, just some remnants of progressbar stuff in koptoptions.lua (#584).

@Frenzie

Frenzie approved these changes Mar 8, 2019

Copy link
Member

Frenzie left a comment

Didn't test but lgtm

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2019

Stuff like setDimensions could also be used on Android, at least in theory (e.g., think of those that can do split screen)

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2019

Well, don't merge yet.
It probably needs some guards because when toggling orientation, onSetDimensions and onChangeScreenMode must both be called, and nested, so you have something like:

03/08/19-12:21:20 DEBUG CreDocument: set bookmarks highlight and internal history true
03/08/19-12:21:20 DEBUG CreDocument: set bookmarks highlight and internal history true
03/08/19-12:21:21 DEBUG CreDocument: set bookmarks highlight and internal history false
03/08/19-12:21:21 DEBUG CreDocument: set bookmarks highlight and internal history false

which feels wrong as the inner one has set it to false while the outer one might still expect it to be true. Probably no impact, but I'd rather have that right.
At least, the crengine re-rendering is done only once.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2019

OK, reworked to avoid duplicating the code, and it seems a bit more logical.
The chain of events was and is:

ReaderRolling:onChangeScreenMode (bottom menu orientation toggle)
  => self.ui:handleEvent(Event:new("SetScreenMode", mode, rotation or true))

ReaderView:onSetScreenMode
  => self.ui:handleEvent(Event:new("SetDimensions", new_screen_size))

ReaderRolling:onSetDimensions
  which now does what the original ReaderRolling:onChangeScreenMode did.

Logs when rotating:

03/08/19-13:08:52 WARN  ReaderRolling:onChangeScreenMode in 111111111111111111111
03/08/19-13:08:52 DEBUG _refresh: Enqueued full update for region 0 0 610 600
03/08/19-13:08:52 DEBUG setDirty full from widget ReaderUI w/ NO region
03/08/19-13:08:52 DEBUG _refresh: Enqueued full update for region 0 0 610 600
03/08/19-13:08:52 DEBUG setDirty partial from widget ReaderUI w/ NO region
03/08/19-13:08:52 DEBUG close widget: table: 0x403607f0
03/08/19-13:08:52 DEBUG setDirty via a func from widget nil
03/08/19-13:08:52 DEBUG setDirty nil from widget ReaderUI w/ NO region

03/08/19-13:08:52 WARN  ReaderRolling:onSetDimensions in 22222222222222222222222222
03/08/19-13:08:52 DEBUG CreDocument: set bookmarks highlight and internal history true
03/08/19-13:08:52 DEBUG CreDocument: set view dimen {
    ["h"] = 600,
    ["w"] = 610
}
03/08/19-13:08:53 DEBUG CreDocument: goto page 351
03/08/19-13:08:53 DEBUG _refresh: Enqueued full update for region 0 0 610 600
03/08/19-13:08:53 DEBUG setDirty partial from widget ReaderUI w/ NO region
03/08/19-13:08:53 DEBUG CreDocument: check xpointer in current page /body/DocFragment[8]/body/section/p[48]/text()[1].317
03/08/19-13:08:53 DEBUG CreDocument: set bookmarks highlight and internal history false
03/08/19-13:08:53 DEBUG _refresh: Enqueued full update for region 0 0 610 600
03/08/19-13:08:53 DEBUG setDirty partial from widget ReaderUI w/ NO region
03/08/19-13:08:53 DEBUG CreDocument: check xpointer in current page /body/DocFragment[8]/body/section/p[48]/text()[1].317

03/08/19-13:08:53 WARN  ReaderRolling:onSetDimensions out 2222222222222222222222
03/08/19-13:08:53 WARN  ReaderRolling:onChangeScreenMode out 11111111111111111111

@poire-z poire-z requested a review from NiLuJe Mar 8, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 8, 2019

I'll try to double-check that w/ the gsensor stuff on a Forma tomorrow at the latest, thanks! ;).

But yeah, this indeed looks like a more logical progression ;).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 9, 2019

(Testing this first thing tomorrow!)

@NiLuJe

NiLuJe approved these changes Mar 10, 2019

Copy link
Member

NiLuJe left a comment

LGTM!

On open:

RV: onSetScreenMode     portrait        0       true
RV: onSetDimensions     table: 0x7447e040
RR: onSetDimensions     table: 0x7447e040

On gsensor rota:

RV: onSwapScreenMode    landscape       1
RR: onChangeScreenMode  landscape       1
RV: onSetScreenMode     landscape       1       nil
RV: onSetDimensions     table: 0x75a6e3b0
RR: onSetDimensions     table: 0x75a6e3b0
RV: onSetScreenMode     landscape       1       nil
RV: onSetScreenMode -> Early abort

@Frenzie Frenzie merged commit 658f513 into koreader:master Mar 10, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 10, 2019

Alright, let's get this one into v2019.03.

@Frenzie Frenzie added this to the 2019.03 milestone Mar 10, 2019

@Frenzie Frenzie added the bug label Mar 10, 2019

@poire-z poire-z deleted the poire-z:fix_emu_resize branch Mar 10, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 10, 2019

Thanks for the tests.

Btw, regarding #4752 (comment), it feels it's really in ReaderView:onSetScreenMode() that it has to be fixed - as I don't see any state: whenever orientation is checked for, there's a call to Device.screen:getScreenMode(), including in ui/data/creoptions.lua which seems to work without state, and doesn't really toggle (because alternate = false).

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Mar 11, 2019

@poire-z : But does Device.screen:getScreenMode() actually return the right state in these instances?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 11, 2019

Yes. In my testing, the bottom menu Orientation toggle was always showing the correct value:

name = "screen_mode",
name_text = S.SCREEN_MODE,
toggle = {S.PORTRAIT, S.LANDSCAPE},
alternate = false,
args = {"portrait", "landscape"},
default_arg = "portrait",
current_func = function() return Device.screen:getScreenMode() end,
event = "ChangeScreenMode",

Easy to test: start the emulator (usually some portrait mode, H>W), and toggle fullscreen (so, W>H).

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.