Skip to content
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

Avoid exposing Pointer and PointersIter from ndk #122

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

fornwall
Copy link
Collaborator

@fornwall fornwall commented Sep 25, 2023

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
Copy link
Collaborator

rib commented Sep 25, 2023

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 fornwall force-pushed the pointer-away-from-ndk branch 4 times, most recently from 99f0ad5 to 5560a71 Compare September 26, 2023 06:35
@fornwall
Copy link
Collaborator Author

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?

Copy link
Collaborator

@rib rib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for taking a look at this!

I see you reduced the boilerplate needed for some of the axis_value convenience getters which makes sense.

This all looks good to me.

Having a common place to now put some docs for the API should be good too.

@rib
Copy link
Collaborator

rib commented Sep 26, 2023

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 :)

@rib rib merged commit 219a14b into rust-mobile:main Sep 26, 2023
3 checks passed
@fornwall fornwall deleted the pointer-away-from-ndk branch September 27, 2023 09:27
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