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

ShadowView - invoke onScrollChanged after internal var update #8858

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MikeEA0
Copy link

@MikeEA0 MikeEA0 commented Feb 19, 2024

Overview

ShadowView - invoke onScrollChanged after internal var update

Proposed Changes

Original View.java first sets member attributes (mScrollX, mScrollY) before listener is called. In that way, when listener accesses these variables, they are correctly set.

Original View.java first sets member attributes (mScrollX, mScrollY) before listener is called. In that way, when listener accesses these variables, they are correctly set.
@hoisie
Copy link
Contributor

hoisie commented Feb 19, 2024

Thanks for the PR. I agree that the way Robolectric fakes scrolling is really bad.

If you want realistic scrolling, set the system property robolectric.useRealScrolling to 'true'. This will use the real Android scrolling code. I would highly recommend using it.

https://github.com/robolectric/robolectric/blob/master/shadows/framework/src/main/java/org/robolectric/shadows/ShadowView.java#L1093-L1095

I would be OK with a PR that uses real scrolling by default, unless robolectric.useRealScrolling is set to false.

@utzcoz
Copy link
Member

utzcoz commented Feb 20, 2024

@hoisie IMO, we can accept PRs to improve legacy non-real scrolling logic if someone can do it like this PR. For a normal user, we can recommend them to use useRealScrolling branch.

@hoisie
Copy link
Contributor

hoisie commented Feb 26, 2024

@utzcoz overall I agree, I will test this PR and check for any compatibility issues.

In the past I have attempted to make some improvements to this fake scrolling behavior and have always run into compatibility issues. The number of tests that inadvertently depend on the broken scrolling logic is quite surprising.

@utzcoz
Copy link
Member

utzcoz commented Feb 27, 2024

@MikeEA0 Please run tests locally and fix failed tests.

@hoisie
Copy link
Contributor

hoisie commented Feb 27, 2024

This is the failure:

2024-02-20T04:55:01.6710780Z org.robolectric.shadows.ShadowViewTest > setScrolls_firesOnScrollChanged[19] FAILED
2024-02-20T04:55:01.6711883Z     value of: access$200(...)
2024-02-20T04:55:01.6712395Z     expected: 122
2024-02-20T04:55:01.6712812Z     but was : 453
2024-02-20T04:55:01.6713707Z         at org.robolectric.shadows.ShadowViewTest.setScrolls_firesOnScrollChanged(ShadowViewTest.java:763)
2024-02-20T04:55:01.6714546Z 
2024-02-20T04:55:01.6715067Z org.robolectric.shadows.ShadowViewTest > setScrolls_firesOnScrollChanged[21] FAILED
2024-02-20T04:55:01.6716003Z     value of: access$200(...)
2024-02-20T04:55:01.6716458Z     expected: 122
2024-02-20T04:55:01.6716845Z     but was : 453
2024-02-20T04:55:01.6717722Z         at org.robolectric.shadows.ShadowViewTest.setScrolls_firesOnScrollChanged(ShadowViewTest.java:763)
2024-02-20T04:55:01.6718564Z 
2024-02-20T04:55:01.6719075Z org.robolectric.shadows.ShadowViewTest > setScrolls_firesOnScrollChanged[34] FAILED
2024-02-20T04:55:01.6719974Z     value of: access$200(...)
2024-02-20T04:55:01.6720426Z     expected: 122
2024-02-20T04:55:01.6720798Z     but was : 453
2024-02-20T04:55:01.6721937Z         at org.robolectric.shadows.ShadowViewTest.setScrolls_firesOnScrollChanged(ShadowViewTest.java:763)

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.

None yet

3 participants