rust-mobile / android-activity

Glue for building Rust applications on Android with NativeActivity or GameActivity
238 stars 46 forks source link

Avoid exposing Pointer and PointersIter from ndk #122

Closed fornwall closed 11 months ago

fornwall commented 11 months ago

See #121, which this PR replaces.

I did this as a quick sketch, can check it more carefully tomorrow, but wanted to check for your thoughts on this approach.

What do you think having a single impl, with #[cfg(feature = "game/native-activity")] checks inside the method, instead of mirroring two impl:s? In this case I personally think it reads better, and guarantees exact equality in the public API and its documentation, but I can change it if desired!

rib commented 11 months ago

I suppose I'm not super keen on putting backend specific details into the common src/input.rs code if it's not too tricky to avoid; though I appreciate that it's not great currently that the compiler doesn't ensure that our implementations remain perfectly API compatible across backends.

Although it'll be more verbose overall I can imagine at some point that we could keep the src/input.rs code backend agnostic but still have the compiler help ensure API compatibility by introducing a backend Impl type where we would end up forced to maintain compatibility due to how the common src/input.rs code would call through to the backend API.

Essentially, the problem right now is that since we don't have full coverage unit tests for the input API then it's very possible to break public api compatibility atm.

Maybe as a starting point for the Pointer type we could:

  1. rename the game-activity Pointer type to PointerImpl or similar
  2. add an equivalent type to the native-activity input.rs
  3. Add a common src/input.rs Pointer that would be something like:
#[derive(Debug)]
pub struct Pointer<'a> {
    inner: PointerImpl<'a>,
}

impl<'a> Pointer<a'> {
    #[inline]
    pub fn pointer_index(&self) -> usize {
        self.inner.pointer_index()
    }
   ...
}

whereby we're forced to keep the PointerImpl APIs compatible by the public shim but we still get to keep the implementation under src/<backend>/. The main trade off is that there's more boilerplate.

For reference there is a vague idea that at some point it could be good to try and implement our own RustActivity backend too that we have more control over and so I want to keep in mind how this would look to also add a third backend.

Alternatively though I think it'd be OK if we just introduce a Pointer type in src/native_activity/input.rs that would hide the ndk Pointer type but it would have the issue you've seen that there's nothing atm to make sure we keep the API in sync across backends. Still we could create an issue to follow up here and do something like above for Pointer, KeyEvent and MotionEvent.

fornwall commented 11 months ago

whereby we're forced to keep the PointerImpl APIs compatible by the public shim but we still get to keep the implementation under src//. The main trade off is that there's more boilerplate.

I tried the PointerImpl variant now, what do you think?

I think it looks ok, and it scales nicely to a possible third RustActivity backend. I prefer wrapping instead of mirroring - especially once function documentation is polished up, it's nice to have that in only one place.

Regarding a possible future third backend - do you envision that it (if it works out nicely) could be the best way forward in the long run, perhaps replacing NativeActivity and GameActivity?

rib commented 11 months ago

Regarding a possible future third backend - do you envision that it (if it works out nicely) could be the best way forward in the long run, perhaps replacing NativeActivity and GameActivity?

NativeActivity is a bit special because that's something that's really supported out of the box in the Android SDK and so uniquely makes it possible to write native applications without needing to write any additional Java/Kotlin code which will probably be reason enough to always maintain support for that.

Hypothetically though, I could imagine that a RustActivity backend could eventually make the GameActivity backend redundant, and unless there would be a strong interest in supporting apps that really subclass GameActivity from Google then I could imagine dropping the GameActivity backend.

Since adding the GameActivity backend I've had to workaround a surprisingly large number of bugs in the C/C++ code and some of those have been things that I think Rust would have avoided. Even recently for example I filed this data race issue: https://issuetracker.google.com/issues/294112477 and so I certainly wouldn't say I have a strong attachment to keeping the GameActivity backend :)