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

VIDEO-5531: Support Video Content Preferences #164

Merged

Conversation

ceaglest
Copy link
Contributor

@ceaglest ceaglest commented Jul 30, 2021

Adding support for Video Content Preferences. The default (resolves to auto) automatically requests appropriately sized video from the server based upon the size of VideoView.

Similar to CTSO the manual setting doesn't call the manual control APIs (yet). In this case you will get the server default for bandwidth and resolution based upon track priority.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

ceaglest added 2 commits July 29, 2021 12:44
* Ability to select VCP in settings
* ConnectOptionsFactory applies the settings
* SettingsStore provides storage / default value
@ceaglest ceaglest changed the title Task/video 5531 video content prefs VIDEO-5531: Support Video Content Preferences Jul 30, 2021
@ceaglest ceaglest marked this pull request as ready for review August 2, 2021 14:39
Copy link
Contributor Author

@ceaglest ceaglest left a comment

Choose a reason for hiding this comment

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

Adding self review of the PR.

@@ -106,7 +106,7 @@ PODS:
- Nimble (9.2.0)
- PromisesObjC (1.2.12)
- Quick (3.1.2)
- TwilioVideo (4.5.0-rc1)
- TwilioVideo (4.5.0-rc4)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needs fixing before merge. I am using the https remote and this branch is setup to use the git remote.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool shouldn't hold up this PR, just the feature branch.

@@ -327,72 +328,6 @@ class MockAppSettingsStore: AppSettingsStoreWriting {
}
}

var invokedLowRenderDimensionsSetter = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not opposed to adding test coverage for the new AppSettingStore properties but I would need some guidance on how to complete this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to test the settings properties in this app. The main thing is that other parts of the app that we may want to add test coverage for and depend on settings are testable. And that's what this mock is for.

If you are curious about Swift mocks you can install this Xcode plugin and then use it to regenerate this mock: https://github.com/seanhenry/SwiftMockGeneratorForXcode

It's kind of cool. But if you have any issues just let me know and I can always run the mock generator and push to this branch. Don't want this to take your time.

@@ -1,5 +1,5 @@
//
// Copyright (C) 2020 Twilio, Inc.
// Copyright (C) 2021 Twilio, Inc.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't done as a git rename - github is getting cheeky.

@ceaglest ceaglest requested a review from timrozum August 2, 2021 14:42
Copy link
Contributor

@timrozum timrozum left a comment

Choose a reason for hiding this comment

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

Lets just figure out the mock changes one way or another and this will be good to go.

@@ -106,7 +106,7 @@ PODS:
- Nimble (9.2.0)
- PromisesObjC (1.2.12)
- Quick (3.1.2)
- TwilioVideo (4.5.0-rc1)
- TwilioVideo (4.5.0-rc4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool shouldn't hold up this PR, just the feature branch.

Comment on lines -34 to -36
var lowRenderDimensions: VideoDimensionsName { get set }
var standardRenderDimensions: VideoDimensionsName { get set }
var highRenderDimensions: VideoDimensionsName { get set }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great.

@@ -327,72 +328,6 @@ class MockAppSettingsStore: AppSettingsStoreWriting {
}
}

var invokedLowRenderDimensionsSetter = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to test the settings properties in this app. The main thing is that other parts of the app that we may want to add test coverage for and depend on settings are testable. And that's what this mock is for.

If you are curious about Swift mocks you can install this Xcode plugin and then use it to regenerate this mock: https://github.com/seanhenry/SwiftMockGeneratorForXcode

It's kind of cool. But if you have any issues just let me know and I can always run the mock generator and push to this branch. Don't want this to take your time.

@timrozum timrozum merged commit 3a3baa8 into feature/media-optimization Aug 4, 2021
@timrozum timrozum deleted the task/video-5531-video-content-prefs branch August 4, 2021 18:38
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