-
Notifications
You must be signed in to change notification settings - Fork 52
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
Not working with Scenes #102
Comments
Is there any data inside |
So when I do |
Can you replicate on a clean project and link it here? This is a tricky one to debug |
Yes. I have just tried to set up the example project, but when I do
I can see in package.json it has |
It should work out of the box, seems like a possible issue with your dev environment |
Try yarn instead of npm |
Ok yarn worked, thanks. |
Update - I got this running, by deleting and cloning from scratch, and this time not doing I then converted the AppDelegate to Swift. And what do you know, it just works still. Flip back to my app, and it doesn't. So I'm baffled, and now on a mission to compare the two. |
Seems like a specific issue with your app, unfortunately. I'll leave this open for now, but if/when you find a solution, it would be very appreciated if you could add a comment with it in case other people have the same issue you're having in the future :) |
Thanks for leaving it open, I will definitely give an update once I land on the reason. |
Ok I have landed on the cause through a process of elimination, but I don't yet understand why it's happening. It stops working at the point you introduce "scenes" to AppDelegate. In order to support CarPlay, you have to have separate scenes for Phone versus CarPlay. If you delete the CarPlay scene and move the code from the Phone scene back into the main AppDelegate, Siri Shortcuts start working again - getInitialShortcut returns an object of info rather than null. Is that enough information to investigate with? Are there any known incompatibiltiy issues with Scenes for this package? |
I don't know, that's a very specific use case that's arguably out of the scope of this package. I would rather not blow up the scope of this package in order to support another third party package. If you do figure out the root cause, and a solution, then a PR detailing the why and how would be appreciated. I don't currently have the time to modify the library for that use case, so that would be something you'd need to figure out on your own and possibly send upstream here for other people. |
I haven't explained it very well, but it's actually not specific to the CarPlay package. It happens when you simply make use of Scenes, and you only need one scene for it to happen. I will share a fork which demonstrates this asap. |
Ok I've got this together. Two branches:
I'm not sure if there's a better way of sharing these or demonstrating the differences? |
Thank you for the example project. Unfortunately, as I've said, I barely have time to maintain this repo with bug fixes / support for new RN releases, so I don't have the time to debug issues for a very specific use-case. If you want to debug this and try to reach a conclusion/solution on what the implementation change needs to be to support this use-case, the best place to start looking is here. This is the method that handles a Edit: I will leave this open until a solution is found or your research deems this as impossible to implement. Hopefully the former |
Thanks. I think I have found the issue, it's described here: https://nemecek.be/blog/20/ios-13-launchoptions-always-nil-this-is-the-reason-solution |
This is also a helpful article for explaining the separation of AppDelegate and SceneDelegate: So far I'm struggling to:
|
I have my doubts that a solution can be achieved here. The API for Scenes seems completely different from the AppDelegate API, and not even React Native is fully geared up to support it (as you've seen with the limitation on storing that information in the JS bridge) |
Well good news. I have got this working! I would like to share the solution with you, but I'm wondering what the best form for a PR would be. As the AppDelegate is now in Swift, it wouldn't make sense to modify the existing example - I think there needs to be a separate Swift/Scenes example. It can be a duplicate of the existing example project, just with the necessary changes. The problem with this approach is you won't be easily able to see the differences in a PR. But I'm not sure of an alternative? In terms of changes to the actual Siri Shortcut library itself, these are purely additions to ios/RNSSSiriShortcuts.h and ios/RNSSSiriShortcuts.m - so it should be possible to merge these without any problems caused to people who don't need to use Scenes. |
Amazing, thank you for your hard work. A new example project for scenes is perfectly fine, just note on the PR which files in the example project are relevant to these changes. Whenever you can, submit a PR with these fixes please 😄 |
@gavrichards this has been released on v3.2.0. Thanks for all your hard work! As a heads up, I've also gone ahead an extracted the connection options -> launch options bridge initialization logic to it's own convenienve RCTBridge initializer, so you should probably use that instead. You can use it like this: let bridge = RCTBridge(delegate: appDelegate, connectionOptions: connectionOptions) |
Oh that's interesting, thanks. I would have had no idea how to do that, or whether it's a good idea. Thanks! |
Tbf, manually creating launch options from connection options is probably also not the safest idea in general. But in React Native land, we don't really go by the rules lol |
No, I hated having to do that. But just couldn't see another way. The two things are not compatible, but until React Native officially issue a way to pass connection options to the bridge (which I figure once Scenes becomes more widely adopted, they'll have to address), I think it's the only way. |
You say that but they still don't even support multiple windows afaik, and iPad support is non-existent pretty much. I do hope they start supporting new APIs, but so far it feels like they're mostly fine with the current implementation and will probably keep it until it's deprecated (if it ever is) |
There is one bug that I couldn't fix with this work, which is probably worth sharing, although I feel like it's an iOS / CarPlay issue, not this package or React Native. Prerequisites:
Actions:
I think this is an iOS bug, because if you don't connect to CarPlay and just try the Siri command on your phone, connectionOptions has the userActivities and you can handle things on app startup as expected. |
Yeah, that seems like a bug on the OS or API side tbh. Btw, I forgot to add the new example project's folder to .npmignore, so the size of the library in versions 3.2.0..3.2.1 is too big. You should install version 3.2.2 which fixes this issue, sorry about that |
Hi @Gustash If I add a log to the scene delegate's willConnectTo handler:
Then close the app, send a push notification, tap on it, this is what is logged:
But when I try to get the notification within the app, I just get So I think the RCTBridge convenience method needs modifying somehow, to ensure this information still comes through. Would you be able to assist? EDIT: I've added some logs into the RCTBridge modifier and can see it is returning here, when there's push notification info, so this explains ths issue: |
I've had to convert my AppDelegate to Swift, because my radio station app uses react-native-carplay, which now requires use of iOS Scenes which in turns requires a Swift AppDelegate.
Everything is working fine except for react-native-siri-shortcut (it was working fine before this).
I can add a shortcut still, but when I say "Hey Siri, play [name of station]" - the app opens, but
getInitialShortcut
returns null. Same goes for usingaddShortcutListener
while the app is open - the event is never triggered.Here's what AppDelegate.swift looks like. I used Swiftify to convert react-native-siri-shortcut's bit (and others) to Swift.
Then in myapp-Bridging-Header.h I have:
Any thoughts or guidance would be greatly appreciated!
The text was updated successfully, but these errors were encountered: