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

Avoid race between updating stepper and ViewPager instantiating the steps #127

Merged
merged 1 commit into from
May 24, 2017

Conversation

bthrall
Copy link

@bthrall bthrall commented May 22, 2017

It was assuming the view pager has layed out all the steps at the time that it calls back to its listeners,
but that is not always true (Handler.post() is happening before ViewPager has inserted the step fragments into the
fragment manager).

This solution waits for the ViewPager to complete its layout before updating StepperLayout.

…teps.

It was assuming the view pager has layed out all the steps at the time that it calls back to its listeners,
but that is not always true (Handler.post() is happening before ViewPager has inserted the step fragments into the
fragment manager).

This solution waits for the ViewPager to complete its layout before updating StepperLayout.
@codecov-io
Copy link

Codecov Report

Merging #127 into master will decrease coverage by 11.26%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #127       +/-   ##
============================================
- Coverage     46.37%   35.1%   -11.27%     
+ Complexity      113      86       -27     
============================================
  Files            24      24               
  Lines           882     883        +1     
  Branches         85      85               
============================================
- Hits            409     310       -99     
- Misses          409     531      +122     
+ Partials         64      42       -22
Impacted Files Coverage Δ Complexity Δ
...main/java/com/stepstone/stepper/StepperLayout.java 24.34% <50%> (-9.66%) 11 <0> (-6)
...stepstone/stepper/internal/util/AnimationUtil.java 0% <0%> (-57.9%) 0% <0%> (-2%)
.../com/stepstone/stepper/internal/util/TintUtil.java 8.82% <0%> (-47.06%) 2% <0%> (-3%)
...e/stepper/adapter/AbstractFragmentStepAdapter.java 55.55% <0%> (-22.23%) 2% <0%> (-1%)
...epstone/stepper/internal/widget/TabsContainer.java 68.25% <0%> (-14.29%) 13% <0%> (-4%)
...com/stepstone/stepper/internal/widget/StepTab.java 29.57% <0%> (-13.39%) 12% <0%> (-3%)
...epstone/stepper/internal/type/DotsStepperType.java 60% <0%> (-13.34%) 2% <0%> (-1%)
.../stepper/internal/type/ProgressBarStepperType.java 56.25% <0%> (-12.5%) 2% <0%> (-1%)
...com/stepstone/stepper/viewmodel/StepViewModel.java 47.05% <0%> (-11.77%) 2% <0%> (-4%)
...epstone/stepper/internal/type/TabsStepperType.java 65.38% <0%> (-11.54%) 4% <0%> (-1%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cffbfe...abb2c4d. Read the comment docs.

@zawadz88
Copy link
Contributor

Hi,
Thank you for this PR, we'll review it shortly :)
Could you provide steps to reproduce the existing issue so that we can try to write tests against it, please?

@zawadz88
Copy link
Contributor

Ok, so I was able to reproduce this and write some tests against this (I'll add them once this is merged).
Thanks for fixing this!

@zawadz88 zawadz88 merged commit dfe6fbf into stepstone-tech:master May 24, 2017
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