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 conversions for JObject wrapper types #50

Open
wants to merge 78 commits into
base: master
Choose a base branch
from

Conversation

uvlad7
Copy link
Contributor

@uvlad7 uvlad7 commented Dec 30, 2023

No description provided.

@uvlad7
Copy link
Contributor Author

uvlad7 commented Jan 4, 2024

@giovanniberti I also added a fix for Box<[bool]> conversion here.

And I need your advice how to add tests for unchecked conversion too.

@uvlad7 uvlad7 mentioned this pull request Jan 4, 2024
@uvlad7
Copy link
Contributor Author

uvlad7 commented Jan 6, 2024

I implemented Object[] <=> Box<[JObject<'env>]> conversion, but I don't know how to make the following

       #[call_type(unchecked)]
        pub extern "jni" fn userArray() -> Box<[User<'env, 'borrow>]> {
            vec![].into_boxed_slice()
        }

work, because it's not possible to impl foreign trait for slices and it's also not possible to impl Signature for Box<[T]>.

@giovanniberti can you take a look, please? I really need this feature to implement some bindings (it actually doesn't matter if rust type is Box<[T]> or something else, but I need to be able to accept arrays as parameters and return them from rust)

UPD: I created conversions for one-dimensional arrays, but not for other types. I'm wondering how to impl TryFromJavaValue for structures like struct UserArray(Box<[User]>) where From<Box<[User]>> for UserArray is implemented. Obviously, in that case we can construct UserArray from java value, but I don't understand how to implement this.

UPD2: I created an example in the tests, but I think it'd be better to impl it with derive for such a wrapper types, and without constcat dependency and this additional ArrSignature trait.

@uvlad7 uvlad7 marked this pull request as draft January 8, 2024 18:09
@uvlad7
Copy link
Contributor Author

uvlad7 commented Jan 11, 2024

@EliseChouleur So, I assume it's better to wait for #59, right?

@uvlad7
Copy link
Contributor Author

uvlad7 commented Mar 19, 2024

@giovanniberti , @EliseChouleur , take a look, please. There are still a lot of improvements to be added, but I assume it'd be better to merge this to fix bugs and to avoid conflicts in migration to jni-0.21, since this PR is really huge.

@giovanniberti
Copy link
Owner

Sorry for the very late review. (understatement of the century right there :) )
First of all: impressive work! You did manage to upgrade jni to 0.21, I tried that a while ago and gave up because of the API changes

Just a few comments:

  1. There are a few commented lines here and there, I'd eliminate those if not needed
  2. There are a couple new traits related to array handling but they don't seem to be documented, could you add a few lines on those?

Thank you again for all the effort!

@uvlad7
Copy link
Contributor Author

uvlad7 commented Sep 25, 2024

Uh, sorry, I created a mess with branches, and unfortunately I didn't manage to update jni to 0.21.1, moved to another branch and gave up. This MR contains other useful fixes, though, and I can return it back into working condition - with jni 0.20 as a dependency - if you want to merge.
I want to continue my efforts in migration to 0.21, but I won't have any time for that in the nearest future

@giovanniberti
Copy link
Owner

This MR contains other useful fixes, though, and I can return it back into working condition - with jni 0.20 as a dependency - if you want to merge.

Sure!

I want to continue my efforts in migration to 0.21, but I won't have any time for that in the nearest future

No problem, I myself have left this repo in a semi-abandoned state since last year — it's in the spirit of FOSS that people can contribute whenever they want and can :)

@uvlad7
Copy link
Contributor Author

uvlad7 commented Oct 5, 2024

I reverted stuff back to jni 0.20.0 version, looks like it's ready to be merged.

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