Merged
Conversation
DataTriny
requested changes
Feb 23, 2025
Member
DataTriny
left a comment
There was a problem hiding this comment.
Very nice work! There are probably too much back and forth when populating nodes and we will likely have to optimize this part, but I'm happy with shipping something that works and improve later.
- Please make sure to implement
Debugon public facing types. - On the documentation of structs, have the first paragraph be short and separate it from the rest with a blank line so that it don't clutter the documentation pages.
- You'll likely want to update the copyright year on all files, sorry about that.
- Don't forget to update the main README. as well as the common crate one.
Member
|
CI steps to enforce code formatting on the Java side and check the dex file must also be added. Checking the dex file is particularely critical. |
6599bae to
e76d780
Compare
Member
|
I think it would make sense to merge the Java formatting step with the existing fmt job. It's wasteful to spin a container and clone the repo again just to check one file. Feel free to rename the steps in the job though, so that it's clear what is going wrong in case of failure. |
…_winit Android backend
…tangle. But NativeActivity seems to be sabotaging this.
…k text traversal problem, which now appears to be a bug in TalkBack. But also be more consistent about only exposing text selection when the node is focused, because that does seem to be what TalkBack expects.
… design rationale
…ot trying to work with NativeActivity
…n't try to make the examples runnable on Android using cargo-apk
…ger than I remembered; update the documentation to reflect that
db8cd13 to
cdb29d2
Compare
DataTriny
approved these changes
Feb 24, 2025
Member
DataTriny
left a comment
There was a problem hiding this comment.
Congratulations on this one!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think the Android adapter is now complete enough to start reviewing.
My current main test case and demo for this adapter is this sample project, which uses a slightly modified version of
GameActivity: https://github.com/[mwcampbell/xilem-to-do-mvc-android](https://github.com/mwcampbell/xilem-to-do-mvc-android)We may debate the division of responsibilities between the native and Java code. Perhaps it would be better if more things were implemented on the native side. To some extent, I just implemented things on the Java side when it was more convenient to write them in Java because of the number of Java method calls required. But I think there's also some logic to my decisions. For example, the current Java implementations of the various event-sending methods post asynchronous requests to the UI thread, but one could imagine an alternate implementation that assumes it's being called on the UI thread and sends the events synchronously. The lower-level
Adaptertype is designed to support such alternate implementations, as mentioned in that types documentation, though no such implementations exist yet (as they'd require a different Android activity class and using something other than winit). I also thought it made sense to store Android-specific state, such as the accessibility focus and current hover (explore-by-touch) node, on the Java side, but am willing to revisit that.I guess we should have CI tasks to verify the Java code formatting and check that the prebuilt
.dexfile matches the source code. I included the script I'm using to build the.dex. It works on my Mac with my particular Android SDK installation. I don't know if it's really necessary to target Java 7, but I remembered reading that Java 8 and above required some tools for desugaring lambdas and possibly other newer features, and I didn't want to try to figure out how to make that work outside of a typical Gradle project. As for whether we should actually use a typical Gradle project, I don't know if that can build a.dexoutside the context of a full Android application.