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: Drop deprecated annotation from native_activity() #280

Merged
merged 1 commit into from
Jun 12, 2022

Conversation

MarijnS95
Copy link
Member

It has been quite some time since we introduced this, and our alternative implementation for more safely passing a native activity to the user while interoperating cleanly with ndk_context still hasn't found its way.

There are legitimate uses for native_activity() that cannot be performed through the suggested ndk_context::android_context().vm(), making the load of this deprecation warning heavier than the problem it tries to prevent.

@MarijnS95 MarijnS95 requested a review from dvc94ch May 25, 2022 16:30
@dvc94ch
Copy link
Contributor

dvc94ch commented May 25, 2022

Can you provide an example? Unless you're building a windowing library like winit, your code will be non portable. Still don't quite understand the use case. There is nothing that isn't specific to the NativeActivity that can't be done via the jni.

@MarijnS95
Copy link
Member Author

I want to access the path variables that Android has already stored for me in NativeActivity, without going through the hassle of JNI or using app_dirs2 (which I finally fixed the broken SharedData/SharedConfig retrieval for: app-dirs-rs/app_dirs2#20).

At the same time I want to access new features like set_window_flags() (#278), which I have yet to come up with a winit binding for nor know if I can fully map all the flags there.

Finally it's just inconvenient for windowing library writers like winit to have to allow deprecated-but-not-really function in their codebase, especially when starting to use use newer window/activity functions on that side.

Since my approach suggested in #211 has effectively been shot down as being too complicated (pass NativeActivity into the function wrapped/called by ndk_glue::main, then force the user to pass that into winit somehow, and remove ndk_glue::native_activity() and native_window() altogether) the only alternative is storing pointers to those objects in ndk-context too. Was there a particular reason why we didn't/don't/can't do that? I don't remember 😅

@MarijnS95
Copy link
Member Author

@dvc94ch Yet another idea - albeit a stupid one because it affects ndk instead of ndk-glue - move the deprecation markers to NativeActivity::vm() and ::activity().

@dvc94ch
Copy link
Contributor

dvc94ch commented May 27, 2022

I want to access the path variables that Android has already stored for me in NativeActivity, without going through the hassle of JNI or using app_dirs2

using something like app_dirs2 is really what we should be incouraging. it seems like a weird place to micro optimize. we can remove the deprecation warning if you feel strongly about it. I believe I added it because you suggested it?

@MarijnS95
Copy link
Member Author

I think you suggested the deprecation attribute, I originally disapproved of it, you removed it, then I requested it again in light of master...MarijnS95:anativeactivity-instance removing static globals...

As per the PR description that branch isn't anywhere near landing, so I retract my recommendation/request for deprecating it 😬 - I think there are valid use-cases for this function, users should just be careful using it.

We have this problem anyway. In a recent test project I didn't call native_activity() myself, but accidentally paired ndk-glue 0.6 with winit 0.26 and wondered why no events were arriving. That can only be solved with the suggested branch above, it is not something this #[deprecated] attribute guards against.

… doc-comments

It has been quite some time since we introduced this, and our
alternative implementation for more safely passing a native activity to
the user while interoperating cleanly with `ndk_context` still hasn't
found its way.

There are legitimate uses for `native_activity()` that cannot be
performed through the suggested `ndk_context::android_context().vm()`,
making the load of this deprecation warning heavier than the problem it
tries to prevent.

In order to keep some form of documentation for this issue going, it is
now explained in a doc-comment that is also partially replicated on the
other `static` getter functions.
@MarijnS95 MarijnS95 force-pushed the ndk-glue-drop-deprecation branch from aea6c33 to f31eeba Compare June 12, 2022 15:08
@MarijnS95
Copy link
Member Author

And of course I should at least maintain the original deprecation message through a doc-comment. English isn't really with me today, let me know if it reads today 😅

Can we do anything else to mitigate this issue, or make the user aware? How about defining a #[no_mangle] symbol (does it need to be pub?) that carries this message in its name (elaborated in a doc-comment), that could fail linking when two ndk_glues appear in the dependency graph? Only one way to find out, I guess :)

@MarijnS95 MarijnS95 requested a review from dvc94ch June 12, 2022 15:14
Copy link
Contributor

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@MarijnS95 MarijnS95 merged commit bfbb3b5 into master Jun 12, 2022
@MarijnS95 MarijnS95 deleted the ndk-glue-drop-deprecation branch June 12, 2022 18:47
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