-
Notifications
You must be signed in to change notification settings - Fork 626
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
Fixed text input on Android, fixed for older Android versions #7204
Conversation
material
stylematerial
style
I've modified the build script of the android activity backend crate, #4920 can be closed. (I had been using the method provided in this issue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR. I'll need to try it out.
This is great because I don't have much experience with older Android and you seem to know better than me.
Perhaps in order to make the review simpler and if it's not too much to ask, could you split the PR in 3 different PR:
- The material changes
- The Support for old android
- The fix for the text input.
So that we can merge the changes independently and some may be easier than others.
Thanks!
// Try to locate javac and java | ||
let javac_java = env_var("JAVA_HOME") | ||
.map(|home| PathBuf::from(home).join("bin")) | ||
.map(|bin| (bin.join("javac"), bin.join("java"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope this also work on windows without the .exe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have tested it, it works on Windows 10 indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did another test under a non-ASCII path with a space and it works too.
@@ -29,13 +29,13 @@ export global Elevation { | |||
|
|||
export global MaterialPalette { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the change to the material style can be extracted into a different PR?
|
||
for (utf8_index, c) in in_str.char_indices() { | ||
if utf16_counter >= utf16_index { | ||
for (utf8_index, _) in in_str.char_indices() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be implemented with in_str.char_indices().skip(char_index).0
or something like that.
But are you sure UTF-32 is the right encoding in Java? I was under the impression it was UTF-16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I made a mistake. I recovered it according to your original code.
I have recovered the |
I have failed to fix the last bug described in #7203 which is related to Unicode index handling, and will not make further modifications in my branch (and posts in the issue), unless it is really required to do something. I am not really good at Android application development. Support for old Android versions is done by asking Copilot (which had omitted a few things before I found them by myself), checking the documentation, modifying the Java code by myself, then trying to compile the Java helper with PS: I will not use the Copilot anymore. My wrong modification of string index convertion is discovered by myself before your review. |
material
style
Excuse me, the unsafe |
I did one more modification because I think it should be required to do this to preserve safety, in case of the Java code's behavior were to be changed in the future. I apologize to you again for my wrong modification of string index convertion functions, which has been realized and recovered by myself ( Please tell me "splitting the PR is really required" instead of "could you split the PR in 2 different PR" to make me do so. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some bug existing elsewhere in the Skia module may be fixed in the future to eliminate this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifications in get_clipboard()
and set_clipboard()
are needed to prevent them from crashing the application. Others fix support for old Android versions (6.0 to 9.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifications in get_clipboard()
and set_clipboard()
are needed to prevent them from crashing the application. Others fix support for old Android versions (6.0 to 9.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced leaked class object reference in load_java_helper(app: &AndroidApp)
, which isn't supposed to be called repeatedly;
Added env.auto_local
or env.delete_local_ref
for other places where new JNI local references are created. This fixes possible crashing on Android versions older than Android 8.0, which don't allow keeping unlimited amount of JNI local references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest forced pushing https://github.com/slint-ui/slint/compare/c0de25edad9b7d524123d061230a848cdcce594c..a02aeded5cdbc24e4fccd219614b2bd4a7f285f8 removes 2 lines of outdated "safety" comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All files are checked again by myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for fixing all that. This looks sensible to me.
I still want to test it before merging.
Sorry for being so slow to answer, but this is the holiday seasons and I'll have more time after new year.
// FIXME: I don't know why this is the case, but without this, the soft input | ||
// always prompt out on startup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// FIXME: I don't know why this is the case, but without this, the soft input | |
// always prompt out on startup. | |
// Without this, the soft input always prompt out on startup. |
Is it only on some version of android?
Is it because there is a text field with the focus on startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I tested an application without any text box, without this modification, the virtual keyboard isn't shown on Android 11.0, but it's shown on Android 6.0 and 8.0. I haven't tested Android 9.0 and 10.0 yet.
I did test the PR locally and it seemed to work. Thanks for your contribution! |
Fixes #7182 and part of #7203.
Note: #4973 may be closed by checking this version of Android build tools: #6695.