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

Add a way to easily convert an AndroidAppWaker into a Waker #171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Nov 11, 2024

This commit adds a "waker()" method to AndroidAppWaker. It converts it
into an std::task::Waker, which is the type of waker used by
asynchronous tasks for scheduling. The goal is to allow AndroidAppWaker
to be easily used to set up an asynchronous context.

The implementation is very efficient, owing to the "static pointer"
style of coding already used. The "wake" function just calls
ALooper_wake, and cloning/dropping the waker is just a copy.

Discussion questions:

  • Should waker() take &self or self? I chose the latter.

This commit adds a "waker()" method to AndroidAppWaker. It converts it
into an `std::task::Waker`, which is the type of waker used by
asynchronous tasks for scheduling. The goal is to allow AndroidAppWaker
to be easily used to set up an asynchronous context.

The implementation is very efficient, owing to the "static pointer"
style of coding already used. The "wake" function just calls
ALooper_wake, and cloning/dropping the waker is just a copy.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Contributor Author

notgull commented Nov 11, 2024

Build failure is due to codebase rot

@MarijnS95
Copy link
Member

#164 (comment)

Yeah, we're really lacking maintenance here, unfortunately. As always, I'm happy to submit PRs to tackle issues and implement features every once in a while in between other tasks, but need a second maintainer for review and approval.

@rib can you update us on your availability and commitment towards this crate, or should we seek for a third maintainer to fill this review-gap?

@MarijnS95
Copy link
Member

Regarding the PR itself, note that the result of a looper wake is very loosely defined and not to be relied on: #170

If there's any explicit async task spawning and progress monitoring that needs to happen on an ALooper, I'd prefer if we add an abstraction based on file descriptors or callbacks which, unlike waking, can be explicitly handled.

@notgull
Copy link
Contributor Author

notgull commented Nov 11, 2024

Interesting. Does it have a chance of spuriously not waking up the event loop? winit currently assumes it always does.

@MarijnS95
Copy link
Member

@notgull according to their documentation, it would get batched up with other events, such that the event loop is "active" / "awake" but without ever seeing it return WAKE because it got "folded into" the other events.

winit currently relies on Poll::Wake to trigger a user-requested redraw:

https://github.com/rust-windowing/winit/blob/ae4c449670674d8ac0d6d8754caf3fe5f4954c25/src/platform_impl/android/mod.rs#L586-L602

But it also checked these before entering a poll_events(): https://github.com/rust-windowing/winit/blob/ae4c449670674d8ac0d6d8754caf3fe5f4954c25/src/platform_impl/android/mod.rs#L562-L565

With these findings, it'll miss events on redraw_flag and proxy_wake_up, unless we hoist those checks out of the match block for any Poll/Err returned out of poll_events(). Or if we want to be more specific, attach a pipe or eventfd to be guaranteed a consistent result whenever our channel into the Looper signaled anything.

@notgull
Copy link
Contributor Author

notgull commented Nov 11, 2024

I would prefer to just use eventfd.

@MarijnS95
Copy link
Member

MarijnS95 commented Nov 11, 2024

Same preference here, it's much more explicit than pretending every Poll could have a Wake folded into it.

For redraws specifically, there's a preference to handle these via AChoreographer intead, which would bump the API requirements. But that's for a future discussion.

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