Skip to content
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

fix: [react-native-sdk] Adds screen share support on iOS #14929

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rcidt
Copy link
Contributor

@rcidt rcidt commented Jul 18, 2024

Seems like the only reason screen share was not working on RNSDK for ios was because the external api is not present, thus no darwin notification events get received by the JS in order to start the web socket.

In the PR I create a dedicated screen share event emitter module for iOS which listens for darwin notification events and forwards them to the JS side.

I made these changes directly within the node_modules in my main project and it is working fine, so I copied the changes into this fork.

Related issue #14928

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@rcidt rcidt changed the title [react-native-sdk] Adds screen share support on iOS fix: [react-native-sdk] Adds screen share support on iOS Jul 18, 2024
@saghul
Copy link
Member

saghul commented Jul 18, 2024

@Calinteodor PTAL.

@rcidt we already have an external API equivalent on RN SDK, perhaps this could tap into that 🤔

@rcidt
Copy link
Contributor Author

rcidt commented Jul 18, 2024

@saghul can you point me in the right direction?

@rcidt rcidt marked this pull request as draft July 18, 2024 20:43
@rcidt rcidt marked this pull request as ready for review July 18, 2024 21:29
@saghul
Copy link
Member

saghul commented Jul 18, 2024

Here is how a new event was added to the RN SDK: 782d46b

You can borrow some inspiration from there I think.

@rcidt
Copy link
Contributor Author

rcidt commented Jul 18, 2024

@saghul the rnSdkHandlers are just react component props passed to the JitsiMeeting component by the parent app. There's no custom native module performing any work here.

I had to add a new native module to listen to the darwn notifications coming from the Broadcast Extension on the native side, and then forward those notifications via RCTEventEmitter to the JS side.

@saghul
Copy link
Member

saghul commented Jul 18, 2024

Sorry, my wires crossed, ignore my previous message.

We had a similar situation with ongoing notifications on Android. Since we now have basically 2 ways to do the same thing, we could move the current native handling part to JS so the same implementation works for both.

@rcidt
Copy link
Contributor Author

rcidt commented Jul 18, 2024

@saghul that's a fair point

…moves it into a common screen-share middleware to be used by both native and RN sdks
@rcidt
Copy link
Contributor Author

rcidt commented Jul 18, 2024

@saghul I made the suggested changes. I moved the screen share event logic out of the external-api middleware and into a common screen-share middleware to be used by both native and RN sdks.

@rcidt rcidt force-pushed the rnsdk/screen-share-ios branch 2 times, most recently from 0f42a1e to 3970c86 Compare July 19, 2024 14:16
@Calinteodor
Copy link
Contributor

@rcidt I tested your PR using the sample app below.
https://github.com/jitsi/jitsi-meet-sdk-samples/tree/master/react-native
Can you please give it a go as well?

@Calinteodor
Copy link
Contributor

Calinteodor commented Jul 31, 2024

@rcidt I tested your PR using the sample app below. https://github.com/jitsi/jitsi-meet-sdk-samples/tree/master/react-native Can you please give it a go as well?

@rcidt I also tested screen-share feature on our end and it seems that some of the changes here broke it.
Please take another look.

}

RCT_EXPORT_MODULE();

- (NSDictionary *)constantsToExport {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check this warning

Module ScreenShareEventEmitter requires main queue setup since it overrides constantsToExport but doesn't implement requiresMainQueueSetup. In a future release React Native will default to initializing all native modules on a background thread unless explicitly opted-out of.

[self handleBroadcastStopped];
}

- (void)handleBroadcastStarted {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread 1: "Error when sending event: org.jitsi.meet.TOGGLE_SCREEN_SHARE with body: {\n enabled = 1;\n}. RCTCallableJSModules is not set. This is probably because you've explicitly synthesized the RCTCallableJSModules in ScreenShareEventEmitter, even though it's inherited from RCTEventEmitter."

@rcidt
Copy link
Contributor Author

rcidt commented Jul 31, 2024

@Calinteodor thanks for the review. I have tested this using the rnsdk but have not done so with the ios sdk. I'll find some time this week to give it a go and look at these issues.

@rcidt
Copy link
Contributor Author

rcidt commented Jul 31, 2024

@rcidt I also tested screen-share feature on our end and it seems that some of the changes here broke it. Please take another look.

Did you test this using the react-native example app, if so what feature is broken?

Or did you test using the ios sample app?

@Calinteodor
Copy link
Contributor

Calinteodor commented Aug 1, 2024

@rcidt I also tested screen-share feature on our end and it seems that some of the changes here broke it. Please take another look.

Did you test this using the react-native example app, if so what feature is broken?

Or did you test using the ios sample app?

I tested your changes on Jitsi Meet. I mentioned the error above on handleBroadcastStarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants