Change the "Active" struct to be sound

[notgull]

The previous implementation is unsound in several cases. This PR ensures that window handles don't outlive the application.

Draft until I can verify that it actually works. Supersedes #114

[notgull]

Looks like the glutin example works on Android, at least from a glance. I think we're ready to start merging and releasing this.

[Lokathor]

Just having a look on the bus, things seem pretty good.

I'd like for there to be more documentation, perhaps at the module level, about how a person writing a system lib should approach things (eg: winit/sdl2 folks), and how a person writing a graphics lib (eg: wgpu/ash folks) should approach things. I think the ideal is that people other than those two groups shouldn't have to look too carefully in here? Just pass a winit thing to a wgpu thing? So the system lib audience and graphics lib audience seem like our main users.

I wouldn't say that this is a blocking concern, but "good docs" is the second most important part of a crate.

[Lokathor]

I'm gonna approve this now, and leave it up to you notgull on if you want to add more usage docs now or not. The alternative is we just merge it and we can always add more docs as a patch if people end up having specific questions.

[notgull]

I'd like for there to be more documentation, perhaps at the module level, about how a person writing a system lib should approach things (eg: winit/sdl2 folks), and how a person writing a graphics lib (eg: wgpu/ash folks) should approach things. I think the ideal is that people other than those two groups shouldn't have to look too carefully in here? Just pass a winit thing to a wgpu thing? So the system lib audience and graphics lib audience seem like our main users.

Sounds good! I've added some more documentation, with some specifically targeting winit/wgpu developers.

[Lokathor]

I think for the doc links to work out approximately correctly you'll have to put in links to https://docs.rs/crate_name (or make them not links)

[rib]

heya, just continuing to follow the trail of recent changes here...

Just because I see that Glutin was used to smoke test this change above - does that mean that you were seeing a problem with Glutin on Android before?

As far as I know, this Glutin example had been working for me on Android: https://github.com/rust-mobile/rust-android-examples/tree/main/na-winit-glutin (working across suspend / resume and cross-platform too)

The main consideration that was required for Android apps was that they need to re-create render surfaces when they resume. One portable way of handling this is that apps can drop their surface state when they Suspend (only ever happens on Android / iOS) and they can lazily create the render surfaces when they Resume (in Winit, all platforms get a Resume event).

I have to admit I'm currently a bit daunted by the new Active state tracking here and wondering about the implications of the RwLock that's involved in the Active tracking for Android. (without scrutinizing the details, I'm just initially worried about whether this could dead lock an event loop trying to suspend or resume)

I've had an initial look at the draft changes to use this in Glutin but I don't know that I entirely followed how the active status is handled (I just noticed that when window.window_handle() was being queried from Winit, then the code was expecting that it would be active I think:

let raw_window_handle =
        window.as_ref().map(|window| window.window_handle()).transpose().expect("Inactive");

and I saw that various places (like create_window_surface) would return an Err Result I think if a handle wasn't active but does that mean that this change is purely about avoiding accessing a handle while inactive? (i.e. it doesn't introduce special handling of the inactive case to try and hide that state from the app)

My current understanding is that if you want to write an application for Android based on this change then it's still your responsibility to only try and create a surface while 'active' and this doesn't try to make APIs such as Glutin automatically poll the active status and lazily [re]create surfaces (that's what I originally guessed this change might be aiming for).

If that's the case then I'm not currently sure if this change was required if there wasn't really a safety issue with accessing inactive ANativeWindows on Android?

As far as I know an ANativeWindow should remain safe to access so long as you hold a reference - it's just that the BufferQueue behind it isn't going to hand out new buffers for rendering once it's been "terminated".

Again, sorry for being late with asking questions here, since this has already been merged, but it feels like quite a significant change which I think is likely to affect my usage and I'm hoping to get a clearer understanding and also double check what's required here.

[notgull]

Just because I see that Glutin was used to smoke test this change above - does that mean that you were seeing a problem with Glutin on Android before?

No, it was mostly just to verify that nothing serious had been broken by my changes. I would've used softbuffer but it doesn't support Android yet, see rust-windowing/softbuffer#44

I have to admit I'm currently a bit daunted by the new Active state tracking here and wondering about the implications of the RwLock that's involved in the Active tracking for Android. (without scrutinizing the details, I'm just initially worried about whether this could dead lock an event loop trying to suspend or resume)

I agree, this is definitely the "monster in the closet" for the current implementation; it introduces a potential deadlock that wasn't there before (i.e. someone is holding a window handle while waiting for events, while the event loop is blocked waiting for that user to drop a window handle).

I think that the docs explicitly call out the fact that you shouldn't use a native window handle after onNativeWindowDestroyed:

You MUST ensure that you do not touch the window object after returning from this function

[MarijnS95]

@notgull let me know if you need a softbuffer implementation for Android, then I'll prioritize cleaning and merging the promised changes.

Aside that would also be nice to validate the glutin changes against wrapping Java Surface/SurfaceTexture objects in NativeWindow, and using those as "display surfaces" in glutin: https://github.com/MarijnS95/AndroidNativeSurface/blob/master/android_native_surface/src/lib.rs