android: jni 0.22 update, exception checks, support any 'Context'#111
android: jni 0.22 update, exception checks, support any 'Context'#111amodm merged 2 commits intoamodm:mainfrom
Conversation
This addresses some exception-handling hygiene issues in the Android backend, and adds support for non-`Activity` `Context` references (considering that `ndk-context` does not guarantee that it provides an `Activity` reference). With the previous implementation, if any of the JNI calls failed (such as the `find_class` call), that would have thrown an exception which was not automatically caught by the jni 0.21 crate. Since there was no code to ensure exceptions were checked and cleared on failure then the previous implementation would have had the side effect of blocking any further JNI on the same thread until the exception was cleared - but it would not have reported a recognisable error for indicating this side effect. By contrast, with jni 0.22 we can rely on the `attach_current_thread` call to check and clear exceptions from the closure before returning. This change also prepares this crate for a planned change in `android-activity` to initialize `ndk-context` with an `Application` reference instead of an `Activity` reference, ref: rust-mobile/android-activity#228 The fact that `android-activity` currently initializes `ndk-context` with an `Activity` reference is not itself guaranteed by the `ndk-context` crate (it only promises an `android.content.Context` reference) but the previous android backend was depending on receiving an `Activity` reference and would throw an exception if it didn't. Note: Associating `ndk-context` with an `Activity` does not align well with Android application programming model because Android applications may often manage many Activities and it doesn't align with the design of `ndk-context` because the `ndk-context` state can't be re-initialized. Even if ndk-context could be changed to support state updates without a panic it could still be ambiguous to maintain a single global reference to one Activity in an application with multiple or possibly no Activity. The new implementation will continue as it did before in case it does receive an Activity reference, but in case it receives some other kind of `Context` then now adds a `ACTIVITY_NEW_TASK` flag to the `Intent` to indicate that the new Activity is allowed to create a new `Task`. In practice this expected to be irrelevant (and result in identical behaviour) because the URL is most likely to be handled by a separate browser application. This needed to bump the rust-version to 1.85 since that's the MSRV for jni 0.22 This also updates the Android test to be based on android-activity instead of ndk-glue
|
Considering that the CI errors are related to the MSRV bump that introduces new Clippy lints, would you rather see the rust-version update in a separate PR that can be merged before this, or is it fine to look at fixing those lints in this PR too? |
@rib it's ok to include it as part of this PR. |
|
@rib Never mind, saw that it was enabled already. |
|
Thanks, I was away this weekend so thanks for fixing up the CI issues with the MSRV bump and releasing. |
This addresses some exception-handling hygiene issues in the Android backend, and adds support for non-
ActivityContextreferences (considering thatndk-contextdoes not guarantee that it provides anActivityreference).With the previous implementation, if any of the JNI calls failed (such as the
find_classcall), that would have thrown an exception which was not automatically caught by the jni 0.21 crate. Since there was no code to ensure exceptions were checked and cleared on failure then the previous implementation would have had the side effect of blocking any further JNI on the same thread until the exception was cleared - but it would not have reported a recognisable error for indicating this side effect.By contrast, with jni 0.22 we can rely on the
attach_current_threadcall to check and clear exceptions from the closure before returning.This change also prepares this crate for a planned change in
android-activityto initializendk-contextwith anApplicationreference instead of anActivityreference, ref: rust-mobile/android-activity#228The fact that
android-activitycurrently initializesndk-contextwith anActivityreference is not itself guaranteed by thendk-contextcrate (it only promises anandroid.content.Contextreference) but the previous android backend was depending on receiving anActivityreference and would throw an exception if it didn't.Note: Associating
ndk-contextwith anActivitydoes not align well with Android application programming model because Android applications may often manage many Activities and it doesn't align with the design ofndk-contextbecause thendk-contextstate can't be re-initialized. Even if ndk-context could be changed to support state updates without a panic it could still be ambiguous to maintain a single global reference to one Activity in an application with multiple or possibly no Activity.The new implementation will continue as it did before in case it does receive an Activity reference, but in case it receives some other kind of
Contextthen now adds aACTIVITY_NEW_TASKflag to theIntentto indicate that the new Activity is allowed to create a newTask. In practice this expected to be irrelevant (and result in identical behaviour) because the URL is most likely to be handled by a separate browser application.This needed to bump the rust-version to 1.85 since that's the MSRV for jni 0.22
This also updates the Android test to be based on android-activity instead of ndk-glue
For reference, I've tested this with an updated
na-eguiexample on Android which lets you interactively click links that are opened with this crate. I've tested with and without the plannedandroid-activitychange to initializendk-contextwith anApplicationreference instead of anActivityand have found the behaviour to be the same in either case.