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

[TIMOB-19040] :Replace MPMoviePlayerController in Ti.Media.VideoPlayer with AVPlayerViewController #8721

Merged
merged 49 commits into from Nov 14, 2017

Conversation

vijaysingh-axway
Copy link
Contributor

hansemannn and others added 30 commits July 8, 2015 15:52
 into TIMOB-19040

# Conflicts:
#	iphone/iphone/Titanium.xcodeproj/project.pbxproj
… TIMOB-19040

# Conflicts:
#	iphone/Classes/MediaModule.m
#	iphone/Classes/TiMediaVideoPlayer.m
#	iphone/Classes/TiMediaVideoPlayerProxy.m
#	iphone/iphone/Titanium.xcodeproj/project.pbxproj
… TIMOB-19040

# Conflicts:
#	iphone/Classes/MediaModule.m
#	iphone/Classes/TiMediaVideoPlayerProxy.m
@@ -458,6 +423,28 @@ - (void)setAudioSessionMode:(NSNumber *)mode
}
}

- (NSNumber *)audioSessionMode
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vijaysingh-axway You added this API but deprecated it as well? Please clarify here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansemannn Good catch! This method was added by you in your initial change for this ticket. I think it is not needed here as audioSessionCategory is already there. Even variables AUDIO_SESSION_MODE_* are also not defined there. So removed this method. Thanks.

@@ -45,6 +49,7 @@
@property (nonatomic, readwrite, assign) TiColor *backgroundColor;
@property (nonatomic, readonly) NSNumber *playing;
@property (nonatomic, copy) NSNumber *volume;
@property (nonatomic, readwrite, assign) id pictureInPictureEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be NSNumber if public API. BOOL otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it NSNumber.

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Some more issues I found during testing. Please also remove the demos/KitchenSink/Resources/another.mp4 file and add an example to the kitchensink-v2 instead.


// Handle both setting this value on running videos and on creation
if ([[movie player] status] == AVPlayerStatusReadyToPlay) {
[[movie player] seekToTime:CMTimeMake(ourTime, 1)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work. Try to use

[[movie player] seekToTime:CMTimeMake(ourTime, 1) toleranceBefore:kCMTimeZero toleranceAfter:kCMTimeZero];

instead.

@build
Copy link
Contributor

build commented Nov 13, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Generated by 🚫 dangerJS

@interface TiMediaVideoPlayerProxy : TiViewProxy {
@protected
AVPlayerViewController *movie;
MPMoviePlayerViewController *cont;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add the deprecated API again here?

enum {
VideoTimeOptionNearestKeyFrame = 0,
VideoTimeOptionExact,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use NS_Enums.

@hansemannn
Copy link
Collaborator

Please also add unit-tests here, there are none so far. The tests can be pretty straight forward with the test-suite you provided earlier.

@property(nonatomic,readonly) NSNumber* VIDEO_CONTROL_NONE; // No controls
@property(nonatomic,readonly) NSNumber* VIDEO_CONTROL_EMBEDDED; // Controls for an embedded view
@property(nonatomic,readonly) NSNumber* VIDEO_CONTROL_FULLSCREEN; // Controls for fullscreen playback
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this comment.

deprecated:
since: '7.0.0'
notes: This property has been removed for iOS in Titanium SDK 7.0.0 as of the official deprecation by Apple.
removed: '7.0.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are single quotes ok in the docs? We usually use ". Please validate the docs locally again. cc @jawa9000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very few places single quotes are there. I updated to ".

@@ -1265,17 +1277,30 @@ properties:
type: Number
permission: read-only
platforms: [android, iphone, ipad]
- name: VIDEO_LOAD_STATE_FAILED
summary: Indicates that the player can no longer play AVPlayerItem instances because of an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Titanium users have no idea what a AVPlayerItem is. Use "media items" instead.

summary: Video scaling is disabled.
type: Number
permission: read-only
- name: VIDEO_SCALING_RESIZE
summary: Specifies that the video should be stretched to fill the layer’s bounds.
Copy link
Collaborator

Choose a reason for hiding this comment

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

That probably fails validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@hansemannn
Copy link
Collaborator

@vijaysingh-axway Can you please re-publish your branch? It was accidentally merged and I did not test the latest changes so far. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants