-
Notifications
You must be signed in to change notification settings - Fork 30
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
Uniffi React Native Integration #401
Conversation
…ostarCore state to the view
Edit: I've added these. |
Initial version of puck, route and trip progress view. Screenrecorder-2024-12-24-07-28-28-106.mp4I have discover one issue and it's that I don't think we can currently override the current location that MapLibre is using within maplibre-react-native. I'm wondering if @KiwiKilian might know of any available solution here or if it's something that's going to need to be exposed on that libraries side. |
Just glanced at your recent commits, do I understand correctly:
Currently there is no clean way to do this. I'm not sure if we you actually want to override the |
Yes we only want to use the snapped location when we are navigating and if our snappedToRoute options is enable in the FerrostarCore, I think the android version does this using a StaticEngine and inserts that into MapLibre @ianthetechie might be able to explain that a bit better. But I don't think that will probably be the best approach here.
I really like this idea and I agree users might want a different functionality here, is this something that could be extracted here or have to be expose on the maplibre-react-native side? |
MapControl need a bit of work, we need some fonts for the +, -, mute, route and recenter button (I've tried + & - as just normal character but it doesn't look good) will look at extracting some google fonts as I think that's what we are using on Android. We will also need to sort out the camera state mostly for the route overview and recenter. I don't think we can just apply a the navigation camera again to reset like we do on Android. So will have to look at keeping a camera state some where probably a provider that will get updated via the |
Okay that should be everything for this. I even added the example app and got that stored. Feel free to finish the review now @ianthetechie |
Co-authored-by: Ian Wagner <[email protected]>
Co-authored-by: Ian Wagner <[email protected]>
…into react-native
Co-authored-by: Ian Wagner <[email protected]>
|
||
sourceSets { | ||
main { | ||
manifest.srcFile "src/main/AndroidManifestNew.xml" |
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.
This is why you have two AndroidManifests. I think one wants to keep it for better compatibility?
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.
Ah makes sense, I think maybe uniffi-bindgen-react-native generates one and moved the old one not 100% sure.
I've changed that to look at the current.
It also pointed out that I needed to change the package name in the manifest.
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 actually think one wants to have two files:
AndroidManifest.xml
- It has the
package
attribute on<manifest>
tag - Used automatically, even without reference in
build.gradle
, whensupportsNamespace
isfalse
- It has the
AndroidManifestNew.xml
- Doesn't have the
package
attribute on<manifest>
tag, because the namespace is that withingbuild.gradle
- Is only used when
supportsNamespace
istrue
- Doesn't have the
- Everything else is the same in both files
- This is the default setup from React Native Builder Bob
Otherwise you will get the following warning:
Setting the namespace via the package attribute in the source AndroidManifest.xml is no longer supported.
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.
It is explained here: callstack/react-native-builder-bob#431
On the other hand, if you only aim for most recent React Native versions support, it might not be necessary? But then the supportsNameSpace
check should be removed and the one and only AndroidManifest.xml
should not contain a package
attribute?
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 wow this is good to know I'll add those back then.
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.
Ahh, fascinating! Thanks for jumping in on this @KiwiKilian!
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.
Great work; I think just one last question!
|
||
sourceSets { | ||
main { | ||
manifest.srcFile "src/main/AndroidManifestNew.xml" |
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.
Ahh, fascinating! Thanks for jumping in on this @KiwiKilian!
dependencies { | ||
// The version of react-native is set by the React Native Gradle Plugin | ||
implementation("com.facebook.react:react-android") | ||
|
||
if (hermesEnabled.toBoolean()) { | ||
implementation("com.facebook.react:hermes-android") | ||
} else { | ||
implementation jscFlavor | ||
} | ||
} |
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, cool!
#import <React/RCTRootView.h> | ||
|
||
#define TIMEOUT_SECONDS 600 | ||
#define TEXT_TO_LOOK_FOR @"Welcome to React" |
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.
Gotcha. Can leave for now and see how things go as we develop a proper onboarding guide for RN.
…igationUiState correctly
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! This looks good to merge now 🚀
Background
After testing an expo modules approach in this PR #394 . We have found that the work to maintain the record types between the Rust layer and expo modules would be too tendinous. So this PR has an initial implementation of using uniffi-bindgen-react-native to create a similar binding as Android and iOS. One downside of this is we have to implement the views all again just for react-native but this downside is worth it long term definitely since the FerrostarCore-RS is still changing it's model quite frequently.
Implementation overview
Implementation of the core integration is following the current Android implementation quite closely apart from segments where there our missing language features and such. We are using uniffi-bindgen-react-native to create the Rust binding the generated TypeScript types are located in
src/generated
.Current status
Please let me know missing implementations that should be on the list and I will update it.
Moved to be separate issues