Skip to content

Implement synchronous horizontal scroll - #111

Open
zhmylove wants to merge 1 commit into
max-kammerer:masterfrom
zhmylove:dev/zhmylove/syncXScroll
Open

Implement synchronous horizontal scroll#111
zhmylove wants to merge 1 commit into
max-kammerer:masterfrom
zhmylove:dev/zhmylove/syncXScroll

Conversation

@zhmylove

Copy link
Copy Markdown

Addresses #102

Implemented new behaviour parameter: syncXScroll which is disabled by default.
When active, it synchronizes horizontal position of all active pages

val isSwapKeys: Boolean
get() = getBooleanProperty(SWAP_KEYS, false)

val isSyncXScroll: Boolean

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhmylove here you use old Orion's approach to work with properties, it doesn't reflect changes in setting until application restart. Please take a look on DRAW_PAGE_BORDER property below, it defines OBSERVABLE property that listens all changes in settings

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll update this part of code soon

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@max-kammerer Hello! I've pushed new variant. This time it respects settings change just in time

var firstXPos = 0f
activePages.forEach {
if (it.isOnScreen) {
if (first)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change in doScrollAndDoRendering is clear, but this one is not. Could you please elaborate it?

@zhmylove zhmylove Jan 4, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that orion-viewer does something like prefetch of the pages: it loads around 3 extra pages to the direction of scrolling and changes in doScrollAndDoRendering properly sync their X-position.

The problem was with other pages being loaded during scrolling. I failed setting their X-position in uploadPrevPage and uploadNextPage, nor inside addPageInPosition — those functions just ignored it and new pages (4+ pages below) got improper position. So the only solution I came across is to synchronize X-position for all the pages being rendered in renderVisiblePages

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will think about it

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was with other pages being loaded during scrolling. I failed setting their X-position in uploadPrevPage and uploadNextPage, nor inside addPageInPosition — those functions just ignored it and new pages (4+ pages below) got improper position. So the only solution I came across is to synchronize X-position for all the pages being rendered in renderVisiblePages

@zhmylove Sorry for long delay in response. It's interesting cause there should be a code that ignores passed value and/or passes default one. Do you have any change to restore from history described above approach and share it via another PR?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I did check my repos and it seems I have no history even in reflog :(

@zhmylove
zhmylove force-pushed the dev/zhmylove/syncXScroll branch from 25f1a56 to 0ef4c10 Compare January 10, 2025 17:24

val DRAW_PAGE_BORDER = pref("DRAW_PAGE_BORDER", true)

val SYNC_X_SCROLL = pref("SYNC_X_SCROLL", true)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 'false' as default here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, changed to 'false'

Signed-off-by: Sergei Zhmylev <zhmylove@narod.ru>
@zhmylove
zhmylove force-pushed the dev/zhmylove/syncXScroll branch from 0ef4c10 to 8acf95e Compare January 11, 2025 11:10
@max-kammerer

Copy link
Copy Markdown
Owner

@zhmylove Would horizontal scrolling help in your scenario? (E.g. #119)

@zhmylove

Copy link
Copy Markdown
Author

@max-kammerer I'm afraid, I do not understand #119 :(

Despite there is no inertness, horizontal scrolling already works properly in Google Play version.

#111 is about making all the pages scroll simultaneously. I mean, I want the whole document scroll whenever I move any it's page as so when I scroll down, all the pages are still aligned

@max-kammerer

Copy link
Copy Markdown
Owner

@zhmylove Currently all pages are organized in vertical structure, #119 suggests to add an option to switch it into horizontal one

//vertical
1
2
3
4
...

//horizontal
1 2 3 4 5 ...

So the prev/next pages would be on left/right side from the current one, not in top/bottom position

@zhmylove

Copy link
Copy Markdown
Author

@max-kammerer ah, I see. Thanks for the clarification!

No, it looks like #111 and #119 cover different use-cases and can not be used interchangeably

@nirodhvana

Copy link
Copy Markdown

@max-kammerer Can this be merged now. This seems cool.

@max-kammerer

Copy link
Copy Markdown
Owner

@nirodhvana I think it requires some additional work

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants