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

ndk-glue: Wrap NativeWindow/InputQueue locks in a newtype #288

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

MarijnS95
Copy link
Member

As recently recommended/requested, this prevents ndk_glue from leaking its underlying lock implementation, and relieves downstream crates from having to import and specify the appropriate lock type. Especially in light of a scheduled migration to parking_lot.

Note that this won't prevent against API breakage introduced by the intended change of transposing the Option inside this LockReadGuard as that can't be done prior to switching to parking_lot, and it'll be its own free-standing API ergonomics improvement regardless (Option now only needs to be unwrapped/checked once).

Finally add some more doc-comments to link to the new lock and explain how the locks should be used on the *Created events and getter functions too, instead of describing it only on the *Destroyed events.

@MarijnS95
Copy link
Member Author

CC @rib.

@MarijnS95 MarijnS95 added difficulty: easy Likely easier than most tasks here priority: normal Great to have blocking a release Prevents a new release priority: high Vital to have type: maintenance Repaying technical debt type: api Design and usability and removed priority: normal Great to have type: maintenance Repaying technical debt labels Jun 10, 2022
@rib
Copy link
Contributor

rib commented Jul 2, 2022

Although I'm not a fan of the current design that requires explicit locking, this patch in itself looks good to me for helping avoid leaking implementation details.

@rib
Copy link
Contributor

rib commented Jul 2, 2022

Something else to consider here is that NativeWindow should perhaps itself implement Clone() in terms of acquiring a reference to the underlying ANativeWindow object which is ref-counted.

Something I was just experimenting with, that's somewhat related to this, was a reference counting wrapper for NativeWindow like:

// XXX: NativeWindow is a ref-counted object but the NativeWindow rust API
// doesn't currently implement Clone() in terms of acquiring a reference
// and Drop() in terms of releasing a reference.

/// A reference to a `NativeWindow`, used for rendering
pub struct NativeWindowRef {
    inner: NativeWindow
}
impl NativeWindowRef {
    pub fn new(native_window: &NativeWindow) -> Self {
        unsafe { ndk_sys::ANativeWindow_acquire(native_window.ptr().as_ptr()); }
        Self { inner: native_window.clone() }
    }
}
impl Drop for NativeWindowRef {
    fn drop(&mut self) {
        unsafe { ndk_sys::ANativeWindow_release(self.inner.ptr().as_ptr()) }
    }
}
impl Deref for NativeWindowRef {
    type Target = NativeWindow;

    fn deref(&self) -> &Self::Target {
        &self.inner
    }
}

And an API for querying the NativeWindow from a separate AndroidApp type I have, something like:

    /// Queries the current [`NativeWindow`] for the application.
    ///
    /// This will only return `Some(window)` between
    /// [`AndroidAppMainEvent::InitWindow`] and [`AndroidAppMainEvent::TerminateWindow`]
    /// events.
    pub fn native_window<'a>(&self) -> Option<NativeWindowRef> {
        let guard = self.native_window.read().unwrap();
        if let Some(ref window) = *guard {
            Some(NativeWindowRef::new(window))
        } else {
            None
        }
    }

I was looking at remove the need to for any global/static API for accessing the NativeWindow.

In my case I'm also not concerned about locking the window a means to block Java from notifying the app about SurfaceView callbacks, so the reference it just about ensuring the pointer would remain valid.

I understand that's not a concern here (since the lock is intended to block the code that might drop the reference and invalidate the pointer) but I figured it was worth mentioning here for future consideration.

@MarijnS95
Copy link
Member Author

Although I'm not a fan of the current design that requires explicit locking

We discussed many times that this is something to be improved in the future, when implementing a unified API over GameActivity + NativeActivity, and supporting multiple Activity instances.

Something else to consider here is that NativeWindow should perhaps itself implement Clone() in terms of acquiring a reference to the underlying ANativeWindow object which is ref-counted.

#207

And the rest of your takes-5-minutes-to-read-wall-of-text becomes moot 😩 😫

was a reference counting wrapper for NativeWindow like...

#309...

I was looking at remove the need to for any global/static API for accessing the NativeWindow.

We discussed this at length as well?

As recently recommended/requested, this prevents `ndk_glue` from leaking
its underlying lock implementation, and relieves downstream crates from
having to import and specify the appropriate lock type.  Especially in
light of a scheduled migration to `parking_lot`.

Note that this won't prevent against API breakage introduced by the
intended change of transposing the `Option` inside this `LockReadGuard`
as that can't be done prior to switching to `parking_lot`, and it'll be
its own free-standing API ergonomics improvement regardless (`Option`
now only needs to be unwrapped/checked once).

Finally add some more doc-comments to link to the new lock and explain
how the locks should be used on the `*Created` events and getter
functions too, instead of describing it only on the `*Destroyed` events.
@MarijnS95 MarijnS95 merged commit 8526787 into master Jul 8, 2022
@MarijnS95 MarijnS95 deleted the ndk-glue-lock-newtype branch July 8, 2022 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking a release Prevents a new release difficulty: easy Likely easier than most tasks here priority: high Vital to have type: api Design and usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants