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

Add support for full screen sharing under iOS #58

Merged
merged 4 commits into from
Feb 24, 2021

Conversation

jcamins
Copy link
Contributor

@jcamins jcamins commented Feb 22, 2021

This PR adds support for sharing the user's full screen under iOS using ReplayKit (with an appropriate app extension). There are still changes that need to be made in the xcode project for the app in order to enable it (documented in docs/IOS-SCREENSHARE.md), but these changes allow the library to be used to enable screen sharing without any further modification of the library code.

@@ -239,4 +243,24 @@ - (void)onMeetingError:(MobileRTCMeetError)errorCode message:(NSString *)message
meetingPromiseReject = nil;
}

- (void)onClickShareScreen:(UIViewController *)parentVC {
if (@available(iOS 12.0, *)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section prefills the iOS SystemBroadcastPicker (accessible from the button in the Control Center) with the extension the developer has implemented as part of the same xcode project.


- (BOOL)respondsToSelector:(SEL)aSelector
{
if (aSelector == @selector(onClickShareScreen:)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no screenShareExtension has been configured, we will report to Zoom that our MobileRTCMeetingServiceDelegate doesn't support onClickShareScreen:.

Copy link
Owner

@mieszko4 mieszko4 left a comment

Choose a reason for hiding this comment

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

looking good thnx.
If I understood correctly, adding MobileRTCScreenShare.framework is still optional if we do not want to integrate screen-share, right?

Comment on lines 19 to 20
appGroupId: 'group.com.your.Bundle',
screenShareExtension: 'com.your.Bundle.ScreenShare'
Copy link
Owner

Choose a reason for hiding this comment

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

  1. since these are only ios-related I think it would be good to prefix them, something like:
Suggested change
appGroupId: 'group.com.your.Bundle',
screenShareExtension: 'com.your.Bundle.ScreenShare'
iosAppGroupId: 'group.com.your.Bundle',
iosScreenShareExtensionId: 'com.your.Bundle.ScreenShare'
  1. also note how I added Id in the second param

@jcamins
Copy link
Contributor Author

jcamins commented Feb 23, 2021

Updated to prefix the two parameters. And to confirm your understanding, if you don't add the MobileRTCScreenShare framework, you won't be able to share your screen, but the meeting will still work fine (if you specify an iosScreenShareExtension in that case, it'll show a "Screen" option under "Share Content" that doesn't work, but I can't come up with any way to avoid this).

[Edited because apparently my brain has melted and I can't spell]

@mieszko4
Copy link
Owner

@jcamins great, thnx! Jus one more note regarding naming: you use iosAppGroupId and iosScreenShareExtension.
We should use iosScreenShareExtensionId for consistency, no?

@jcamins
Copy link
Contributor Author

jcamins commented Feb 24, 2021

Whoops, sorry. You even pointed that out before. Updated.

By the way, it looks like you're only one who can merge PRs into this repo, so I'm going to assume now that I've addressed the comments these are good to go. Let me know if you need anything else!

@mieszko4
Copy link
Owner

Oh, ok, I thought that once I approve you can merge. Anyway I will merge it now, thnx!

@mieszko4 mieszko4 merged commit 56a578c into mieszko4:master Feb 24, 2021
@jcamins jcamins mentioned this pull request Feb 24, 2021
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.

2 participants