Skip to content

Conversation

@tkashkin
Copy link
Contributor

Do not throw IllegalStateException when x is out of bounds and return last button instead

Do not throw IllegalStateException when x is out of bounds and return last button instead
@addisonElliott
Copy link
Owner

Thanks for the PR. I'll try to test this sometime this week.

I'm not sure this is the best course of action to take if the X position is out of bounds. My concern is that what if the touch event bounds given is not the last button and the button jumps a bit because the touch event causes it to move.

Rather, would it be better to return -1 (or NaN for float) and ignore the touch event?

What are your thoughts?

@tkashkin
Copy link
Contributor Author

I'm not sure either, but this should happen only if user starts dragging and continues to drag to right until x becomes out of bounds. It seems appropriate to return last button in this case.

If user drags to left it will always return first button because x <= button.getRight() will always be true.

I haven't noticed any side effects of this fix.

@addisonElliott
Copy link
Owner

Okay, maybe this is the best course of action then.

It's odd because in theory, the touch events should not be generated if out of bounds. When I had this problem before, the error was because the bounds were not relative to the segmented button group but rather they were coming from a parent ScrollView which was messing up the X coordinate.

If that's not what is happening here then we should be good.

You're welcome to do this if you like, but I'll probably take your test case given in #10 and put it in the sample library. Then I'll make sure I can reproduce the bug and that it gets fixed with your fix. This will be good for future work to make sure we don't introduce the issue again.

@addisonElliott addisonElliott merged commit 2fefe53 into addisonElliott:master Apr 24, 2019
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.

2 participants