Added android support #40
Conversation
|
@chrisduerr Here it is. But it still needs further checking. |
|
@Fancyflame Do you plan on finishing this up or are you looking for feedback on specific issues? I usually don't do reviews unless the PR is finished and CI goes through. |
|
@chrisduerr Actually I didn't edit anything about Linux, also don't have Linux development experience... Sure, I need some feedback. EDIT: I repeatedly checked my code, everything changed only happens on Android platform. As you can see, any changes is behind |
|
@Fancyflame Check out why the build failed. You need to format the code. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Well, you helped me formatted the code, but I wonder what the configuration is, maybe I'll update these files in the future for optimization. (Wierd that it's not the same as rust playground) |
| let cb = env.get_static_field(class_ctx, "CLIPBOARD_SERVICE", "Ljava/lang/String;")?; | ||
| let cb_manager = env | ||
| .call_method( | ||
| ctx.context() as jni::sys::jobject, |
There was a problem hiding this comment.
There's a bunch of instances here where you have specified multiple path objects. For structs and other types the import should always go up to the last path element, so a use jni::sys::jobject should be used here.
This applies to other areas too like jni::JavaVM for example.
There was a problem hiding this comment.
JObject must live with env, storage JObject is not easy due to the limitation of API form.
src/android_clipboard.rs
Outdated
| let ctx = ndk_context::android_context(); | ||
| let vm = unsafe { jni::JavaVM::from_raw(ctx.vm().cast()) }?; | ||
| let env = vm.attach_current_thread()?; | ||
| let class_ctx = env.find_class("android/content/Context")?; | ||
| let cb = env.get_static_field(class_ctx, "CLIPBOARD_SERVICE", "Ljava/lang/String;")?; | ||
| let cb_manager = env | ||
| .call_method( | ||
| ctx.context() as jni::sys::jobject, | ||
| "getSystemService", | ||
| "(Ljava/lang/String;)Ljava/lang/Object;", | ||
| &[cb], | ||
| )? | ||
| .l() | ||
| .unwrap(); |
There was a problem hiding this comment.
All of this seems duplicated between the two methods. It also seems to repeatedly access system stuff again and again. Can this not be persisted on the AndroidClipboardContext struct?
There was a problem hiding this comment.
About this part, I have considered storage the duplicated part into a struct, but cb_manager here borrows vm, which is related to self-reference. I have made a crate to support self-reference: https://github.com/Fancyflame/Any-ref-rs. Its stability should be fine (code reviewed by rust forum) but it requires a high rustc version. Alternatively, we can merge the duplicate part into a function, which can re-use the code but it will not increase any performance, that is to say it still will access the system repeatedly. I prefer the latter, What do you think?
All you have to do is run |
|
I saw your annotation. But I'm afraid I can only correct them at night (about after 8 hours) because I have classes through out the day. |
Absolutely no worries, take as much time as you want. Just make sure you re-request a review once you're looking for feedback so I know you're waiting for me. |
No description provided.