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

Fix: Color state list now checks all possible states #351

Merged
merged 1 commit into from
May 28, 2020

Conversation

alorma
Copy link
Member

@alorma alorma commented May 28, 2020

Thanks to @BraisGabin on #350 that pointed out that checkColorList_whenDisabled and checkColorList_whenChecked tests were failing.

I've investigated and found that on API 22 color state was not returning the same instance, (I believe android does some caching in future versions).

So I've moved color state list check to verify all possible states of color state list instead of checking for the same instance.

Captura de pantalla 2020-05-28 a las 14 30 18

@alorma alorma requested review from Sloy, rocboronat and a team May 28, 2020 12:30
@alorma alorma force-pushed the fix_colorstatelist branch from 4b7dd60 to 909e157 Compare May 28, 2020 12:33
@alorma alorma merged commit 7005df6 into master May 28, 2020
@alorma alorma deleted the fix_colorstatelist branch May 28, 2020 13:01
return currentColorList == expectedColorInt
val expectedColorList = ContextCompat.getColorStateList(textView.context, expectedColor)!!

val allStates = ALL_COLOR_STATE_LIST_STATES.flatMap { state -> listOf(state, -state) }.map { state -> intArrayOf(state) }
Copy link
Member

Choose a reason for hiding this comment

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

What does this - of listOf(state, -state) mean? Just curiosity

Copy link
Member Author

Choose a reason for hiding this comment

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

It reverses the state:

state enabled: android.R.attr.state_enabled
state disabled: -android.R.attr.state_enabled

Copy link
Member

Choose a reason for hiding this comment

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

WOW, that's a big WTF. What's the magic behind that?

IMHO we could do something easier to catch...

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

@BraisGabin
Copy link
Contributor

Oh! And it was a real error, i wasn't just a flaky test. Nice fix :)

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.

4 participants