-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 for #5634 Crash after zooming map #5700
Conversation
} catch (IllegalArgumentException e) { // https://github.com/getodk/collect/issues/5379 | ||
// https://github.com/getodk/collect/issues/5379 | ||
// https://github.com/getodk/collect/issues/5634 | ||
} catch (RuntimeException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to go with a more specific IllegalStateException
catch here (as you suggest as an alternative in the PR description). RuntimeException
is very broad, and might catch an unrecoverable exception that we'd actually rather have crash the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes, better safe than sorry.
Shouldn't we fix zooming so that it works well for points/traces/shapes instead of catching the exception? It could be a good short-term solution to avoid crashes if we wanted to have a release soon but otherwise it's not a proper solution to me. @seadowg what do you think? |
@grzesiek2010 great point! I think avoiding the crash is a good step forward, but agree that we probably want to add another issue for improving the zoom behaviour. That's probably a good thing to look at after @getodk/testers have had a play with this. |
Tested with Success! Verified on Androids: 10 Verified cases:
|
Tested with Success! Verified on Androids: 13 |
Thanks guys 😀 |
Fixes #5634
What has been done to verify that this works as intended?
Tested manually using issue form.
Why is this the best possible solution? Were any other approaches considered?
The issue seems analogous (even identical) to #5379, with Google's
CameraUpdate
again choking onLatLng
data. Rather than simply updating to handle theIllegalStateException
thrown by this issue, I've generalised toRuntimeException
.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Behaviour is now as in #5393. No impact on tests.
Do we need any specific form for testing your changes? If so, please attach one.
Form provided in #5634.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.