project-robius / robius-android-env

easy Rust access to Android state (native Java objects) managed by UI toolkits
MIT License
1 stars 2 forks source link

Overlap with `ndk-context`? #1

Open MarijnS95 opened 1 month ago

MarijnS95 commented 1 month ago

The ecosystem already has a crate that serves the (as it seems) exact same purpose: https://crates.io/crates/ndk-context. Any reason that isn't used?

Besides, we made the wrong choice in ndk-context that an Activity is a global singleton, and it looks like this crate is making the same mistake.

kevinaboos commented 1 month ago

Hi @MarijnS95, thanks for the question -- I did not know about ndk-context, it does indeed look somewhat similar, so thanks for alerting us to that.

We didn't use existing crates because this crate is designed to directly support multiple different UI toolkits. Currently only Makepad is supported, but we will shortly support Dioxus (and thus, Tauri since they both use wry), as well as others (e.g., Xilem/Linebender org projects) in the future. Each of those UI toolkits naturally manage and expose Android state in their own unique ways, so the goal of this crate is to remove the burden of contending with those differences from the shoulders of the app developer and have it "just work" behind the scenes. Makepad's build system (cargo-makepad) will also soon directly support "activating" this crate during build time too, so the two are closely coupled atm.

Besides, we made the wrong choice in ndk-context that an Activity is a global singleton, and it looks like this crate is making the same mistake.

We do assume a single Activity because that appears to be the model used by Makepad, Dioxus, Tauri, and Xilem, and perhaps others. Note that we do assume and account for the fact that the actual Activity instance may change (when it's destroyed and recreated by the system). Perhaps that model will have to change in the future, but those toolkits do preserve that assumption at the moment. Technically our with_current_activity() fn can be implemented to not make that assumption, i.e., if a UI toolkit wanted to provide a different "current activity" context, but yes, we do assume that there is only one current activity.

I'm actually quite curious as to why that is the wrong choice. Clearly, a UI toolkit may choose to do things differently, but I'm not sure how and when a second activity object would be used, or why.

MarijnS95 commented 1 month ago

The current design for ndk-context already allows compatibility with any UI toolkit, no need to reinvent that (now that you know about its existence). Alas, Android Rust ecosystem divergence seems to be preferred nowadays.


The current implementations and users of ndk-context don't even allow activity replacement. And there's this complicated case where Android creates a new Activity before destroying the old one, temporarily having two. And obviating that at this low level might be very complex.

It'd be much better to have the Application context pointer which should be adequate for most crates that simply need Context.

kevinaboos commented 1 month ago

i did just find this comment from you, which is an interesting case of potential simultaneous multi-activity existence. Will investigate.

kevinaboos commented 1 month ago

Perhaps we could layer this crate atop ndk-context, i'm not sure; I can discuss it with our team. I'm not really sure what benefit that would offer, though.

One issue is that Makepad is incredibly sensitive to dependencies (so we wouldn't be able to depend on something like android-activity, but ndk-context alone should be ok). Other UI toolkits don't seem to care as much.

kevinaboos commented 1 month ago

The current implementations and users of ndk-context don't even allow activity replacement. And there's this complicated case where Android creates a new Activity before destroying the old one, temporarily having two. And obviating that at this low level might be very complex.

robius-android-env does support that "activity replacement" by re-obtaining the curr activity instance upon every call to with_current_activity(), which i have tested when a Makepad app is switched to split screen, resized, or upon another action that causes the system to tear down and recreate the activity. IMHO, this is one benefit of our design compared with the current "set and take" design of ndk-context. But yes, it doesn't support multiple activity objects, right.

It'd be much better to have the Application context pointer which should be adequate for most crates that simply need Context.

For certain platform APIs (e.g., biometric authentication), we need the activity context not the application obj.

kevinaboos commented 1 month ago

The current design for ndk-context already allows compatibility with any UI toolkit, no need to reinvent that (now that you know about its existence).

Looking at it in more depth, the way that ndk-context expects a UI toolkit to directly set, remove, and re-set the android context (or do it via ndk-glue, which is now deprecated in favor of android-activity) presents a burden either to the UI toolkit or the app dev (if the UI toolkit doesn't do it). I can't speak for Dioxus or Xilem, but I'm confident that Makepad will not permit that sort of state-update init functions that always invokes a foreign crate's functions, which will leave the burden on the app dev --- precisely what this crate tries to avoid.

We also tried to make the interface used by the "middleware crates" (as you call them) and/or app crate as safe, limited, efficient, and standalone as possible, hence why this crate only exposes access to those context states via with_current_activity(). Allow me to enumerate:

Alas, Android Rust ecosystem divergence seems to be preferred nowadays.

Definitely not trying to contribute to divergence. I do want to find a way to consolidate and preserve compatibility across the ecosystem, but it seems tough unless you're willing to accept major redesigns to this crate (which may come at no real benefit?).

kevinaboos commented 1 month ago

Another option could be for robius-android-env to depend on ndk-context for winit/egui support, since that's already offered. Not sure, but I'll bring it up with the Linebender org today in our monthly meeting.

MarijnS95 commented 1 month ago

i did just find this comment from you, which is an interesting case of potential simultaneous multi-activity existence. Will investigate.

Yes, once you get into the territory of opening share dialogs or other "pages within your app" via intents in different "task stacks", you might very well have many activity instances open concurrently that aren't just "because of a reconfig" (when resizing the app or switching to floating / windowed / split mode).


Perhaps we could layer this crate atop ndk-context, i'm not sure; I can discuss it with our team. I'm not really sure what benefit that would offer, though.

Putting a stop on driving ecosystem divergence, that would do it for me. As mentioned I've long desired to address this issue, and having an actual user - perhaps even collaborator! - would greatly encourage that.


IMHO, this is one benefit of our design compared with the current "set and take" design of ndk-context. But yes, it doesn't support multiple activity objects, right.

As said the original ndk-context design got it wrong, and it'll have to change to fix this long-standing bug. Any breaking changes or redesign is expected.

It'd be much better to have the Application context pointer which should be adequate for most crates that simply need Context.

For certain platform APIs (e.g., biometric authentication), we need the activity context not the application obj.

We could expose both, is what I'm saying. Certain crates only need "a Context" and don't have to know or care which of the Activitys to use.


Re "state setting now allowed": that's exactly what this crate does for the VM, and implicitly for an "activity getter"? The value for the VM, and the implementation used for the "activity getter", is explicitly defined by the UI / windowing backend.


  • efficient: minimizes the unnecessary conversion to/from jni's higher-level objects and the lower-level raw pointer types. In contrast, ndk-context's API would require "down"-converting JObjects into raw pointers, and then back up to JObejcts by the middleware/library crates.

The versioning problem is specifically why ndk-context tried to do with no dependencies, which is specifically why no API like the jni crate is in publicly exposed crates.

However, you might have also found that I've expressed multiple times that relying on static globals is just a broken design. This is also why winit got changed to use a with_android_app() constructor instead of pulling it from a static global. That same thought jives with the linked fn dispatch() function on wry. More explicit APIs like this protrude hidden-behind-your-back synchronizations via static globals across crate ecosystems.

MarijnS95 commented 1 month ago

tl;dr:

I'd love to collaborate on pulling a generic solution into ndk-context, and push a generic solution to the "Rust Android ecosystem as a whole" that doesn't just benefit robius.

kevinaboos commented 1 month ago

Putting a stop on driving ecosystem divergence, that would do it for me. As mentioned I've long desired to address this issue, and having an actual user - perhaps even collaborator! - would greatly encourage that.

Excellent, let's do it! I'm open to contributing/collaborating on this front for sure. 👍

We could expose both, is what I'm saying. Certain crates only need "a Context" and don't have to know or care which of the Activitys to use.

Great, yes, we'll do that. The custom.rs module in this crate is very close to that already, by coincidence.

The versioning problem is specifically why ndk-context tried to do with no dependencies, which is specifically why no API like the jni crate is in publicly exposed crates.

I see, yes that does make sense. I would probably still prefer to expose jni-defined types through the public API of this crate (for a variety of reasons), but we certainly don't have to require that in the next-gen ndk-context redesign.

kevinaboos commented 1 month ago

tl;dr:

Indeed; given that, I would like to respect that wide usage then, and also become a user as previously mentioned.

  • I like your design for setting a global callback to get the current activity/context (and only for a limited amount of time), instead of storing the lifetime-elided pointer directly;

Sounds good.

  • Would be useful to have the same for the Application context;

Agreed. I'd be happy to add that even before we have a downstream crate driving the need for it.

  • I understand that your design requires you to poke into other crates (makepad), when those crates don't want to "give the state to your crate" in their init;

Not strictly required, but majorly preferred in our view. Of course that shouldn't be a concern of ndk-context and thus shouldn't be a requirement relevant to this redesign.

  • Global statics will be a problem when there's inevitably multiple semver-incompatible releases of robius-android-env (which is inevitable with eg jni 0.x in the public API);

That's fair. I had intended to release it closely with Makepad's release cycle, but yeah that is potentially annoying to deal with.

I'd love to collaborate on pulling a generic solution into ndk-context, and push a generic solution to the "Rust Android ecosystem as a whole" that doesn't just benefit robius.

Same, we're on board. How do you propose to proceed -- would you like to take a first crack at it since you already have many design ideas for the next-gen ndk-context, and then I can review/comment/modify to suit our needs? Or would you prefer me to attempt to rewrite robius-android-env to use ndk-context first, and then try to draft what an ideal next-gen of ndk-context would look like in our view?

kevinaboos commented 1 month ago

we can also discuss on linebender zulip. I will post there.

EDIT: posted a new discussion thread here.

MarijnS95 commented 1 month ago

Awesome, glad you're on board @kevinaboos!

It'll take me some time to dig through android-activity and winit to make them multi-Activity aware, and use that as a test-bed to drive any ndk-context changes from this angle. Are there any time constraints on your end I should be aware of? It's currently well after working hours and close to midnight here, but we could set some kind of deadline where if I didn't get around to it within a certain number of hours or days, you propose your changes to ndk-context first :)

kevinaboos commented 1 month ago

No rush whatsoever; feel free to take your time.

I can also immediately[^1] modify robius-android-env to directly use ndk-context as a replacement for our existing custom.rs module, which signals our intent and mutual agreement to work together. Then, whenever your redesign work is ready, I can modify our crate again to use that next-gen version.

[^1]: somewhat soon, when time permits, but without having to wait on your changes.

kevinaboos commented 1 month ago

I added support for ndk-context in the latest commit: https://github.com/project-robius/robius-android-env/commit/833fb5577a7995dd96e8c3ee19799ffef79cdad4#diff-6599ff4301c89f3873031342f21e0c1672bc7b8092c26530443d9d01dc6f5dd4

kevinaboos commented 1 month ago

haven't tested it yet since i don't have quick & easy access to an example app, but i'm trying to get one running for Dioxus