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

Overlap with ndk-context? #1

Open
MarijnS95 opened this issue May 21, 2024 · 16 comments
Open

Overlap with ndk-context? #1

MarijnS95 opened this issue May 21, 2024 · 16 comments

Comments

@MarijnS95
Copy link

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

kevinaboos commented May 21, 2024

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

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

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

@kevinaboos
Copy link
Member

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

kevinaboos commented May 21, 2024

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

kevinaboos commented May 21, 2024

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:

  • safe: obviously it can't be perfectly safe between the UI toolkit and this crate, but we aim to offer a safe API between this crate and the "middleware"/library crates or the app crate.
  • limited: the interface only provides a reference to the JNIEnv and curr activity JObject, whose lifetime is elided/anonymous, ensuring it is tied to the lifetime of that function only. This is similar to how a thread-local variable cannot escape the lifetime of the function in which it is accessed (contributing to safety, above).
  • 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.
  • standalone: as mentioned above, we want this crate to be usable as a "passive" library, i.e., without first calling any functions that initialize its state. Obviously we can't do that for the manual/custom UI toolkit case just like how ndk-context works, but we can do it for specific UI toolkits.

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

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

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

MarijnS95 commented May 21, 2024

tl;dr:

  • ndk-context is widely used: https://crates.io/crates/ndk-context/reverse_dependencies (even directly by crates you mentioned, such as wry, and of course winit which is the path chosen by Linebender / Xilem);
  • 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;
  • Would be useful to have the same for the Application context;
  • 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;
  • 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);

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

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

kevinaboos commented May 21, 2024

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

kevinaboos commented May 21, 2024

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

EDIT: posted a new discussion thread here.

@MarijnS95
Copy link
Author

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

No rush whatsoever; feel free to take your time.

I can also immediately1 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.

Footnotes

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

@kevinaboos
Copy link
Member

I added support for ndk-context in the latest commit: 833fb55#diff-6599ff4301c89f3873031342f21e0c1672bc7b8092c26530443d9d01dc6f5dd4

@kevinaboos
Copy link
Member

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

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

No branches or pull requests

2 participants