Add from_java() constructors to InputQueue, KeyEvent and MotionEvent
#456
Conversation
MarijnS95
force-pushed
the
event-from-java
branch
from
ab25843 to
2ae4d4f
Compare
MarijnS95
force-pushed
the
event-from-java
branch
from
2ae4d4f to
398180c
Compare
| @@ -6,6 +6,9 @@ use std::io::Result; | |||
| use std::os::raw::c_int; | |||
| use std::ptr::{self, NonNull}; | |||
|
|
|||
| #[cfg(feature = "api-level-33")] | |||
| use jni_sys::{jobject, JNIEnv}; | |||
There was a problem hiding this comment.
I'd be slightly tempted to just use c_void pointers and avoid exposing any jni crate types in the public API but maybe there are other apis already exposing jni types?
The pointers are for unsafe methods anyway so the reduced type safety could be acceptable.
There was a problem hiding this comment.
Sure, let's think about it a bit and do that in a followup
impact: breaking
MarijnS95
force-pushed
the
event-from-java
branch
from
398180c to
a0c84ef
Compare
|
Force-pushed to fix some wrong intradoc links :) |
ndk/src/event.rs
Outdated
| /// # Safety | ||
| /// | ||
| /// This function should be called with a healthy JVM pointer and with a non-null | ||
| /// [`android.view.MotionEvent`], which must be kept alive on the Java/Kotlin side. |
There was a problem hiding this comment.
@rib I copied the which must be kept alive on the Java/Kotlin side. portion from other from_java() functions, but I don't think it holds true for these APIs.
- As the copied doc-comment above suggests:
... that is a copy of the specified Java ...; - And
AInputEvent_release()which must be called states:The underlying Java object remains valid and does not change its state..
It seems the two objects are independent and have their own lifetime after this call.
If you agree I'll remove this text.
There was a problem hiding this comment.
Hmm, I'm not sure tbh - from when I last tried to get my head around the implementation details for input event handling I recall some pretty funky lifetime tracking for events that went via an InputQueue since there's some special callbacks in java for events after they are handled by native code from what I vaguely recall and I don't really know how the bits all fit together if using these without really digging into the implementation.
My guess would be that you do actually need to ensure the event is kept alive in the jvm if they only hold a weak reference in the native event and guess that the release isn't just about freeing the struct but also tracking when the event is finished with on the jvm side so that it can progress to the next stage of being handled in the jvm.
I'd er on the side of keeping that requirement in the docs unless we really look into the implementation details more closely.
There was a problem hiding this comment.
It constructs it here:
Which copies over a bunch of fields:
All information is copied as raw jints by value, and I don't think the various id fields have a back-reference to the Java representation.
Likewise the motion events are fully copied:
With nothing that appears to be a reference.
Let's remove the doc text now that we've confirmed that the spec is in accordance with the implementation.
There was a problem hiding this comment.
and guess that the release isn't just about freeing the struct but also tracking when the event is finished with on the jvm side so that it can progress to the next stage of being handled in the jvm.
To confirm, the terribly named _release (why not _destroy... release sounds like refcounting) function only calls delete and doesn't perform any "event finished" handling later. That, after all, happens elsewhere via AInputQueue_finishEvent() which has a reference to the input queue and exists specifically for explicitness of this purpose.
Looks like I already removed the wording in the last force-push, let's merge this.
MarijnS95
force-pushed
the
event-from-java
branch
from
a0c84ef to
6ab211d
Compare
These were added in API 31 (
KeyEvent/MotionEvent) and API 33 (InputEvent), but not yet mapped in the NDK crate.