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

feat(android): add new constants for video scaling #11148

Merged
merged 8 commits into from Sep 11, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 15, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-27184

Description:
Add

Ti.Media.VIDEO_SCALING_RESIZE
Ti.Media.VIDEO_SCALING_RESIZE_ASPECT
Ti.Media.VIDEO_SCALING_RESIZE_ASPECT_FILL

constants in Android. Added a deprecation message for using the old ones. I suppose we are removing them in one of the next major releases for keeping parity? Also I have not made any changes for in the docs, since now the old constants are marked as removed in all platforms, but are actually working fine on Android. Should we change that to list them as only deprecated on Android?

Note: New constants are related with visual changes, so no unit tests.

Test case:

var vidWin = Titanium.UI.createWindow({
	layout: 'vertical',
    title : 'Video View Demo',
    backgroundColor : '#fff'
});

var videoPlayer = Titanium.Media.createVideoPlayer({
    top : 2,
    autoplay : true,
    backgroundColor : 'blue',
    height : 250,
    width : 250,
    mediaControlStyle : Titanium.Media.VIDEO_CONTROL_DEFAULT,
    scalingMode : Titanium.Media.VIDEO_SCALING_MODE_FILL // mode 1
    //scalingMode : Titanium.Media.VIDEO_SCALING_ASPECT_FILL // mode 2
    //scalingMode : Titanium.Media.VIDEO_SCALING_ASPECT_FIT // mode 3
});
var videoPlayer2 = Titanium.Media.createVideoPlayer({
    top : 20,
    autoplay : true,
    backgroundColor : 'blue',
    height : 250,
    width : 250,
    mediaControlStyle : Titanium.Media.VIDEO_CONTROL_DEFAULT,
    scalingMode : Titanium.Media.VIDEO_SCALING_RESIZE // mode 1
    //scalingMode : Titanium.Media.VIDEO_SCALING_RESIZE_ASPECT_FILL // mode 2
    //scalingMode : Titanium.Media.VIDEO_SCALING_RESIZE_ASPECT // mode 3
});
videoPlayer.url = 'https://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_320x180.mp4';
videoPlayer2.url = 'https://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_320x180.mp4';
vidWin.add(videoPlayer);
vidWin.add(videoPlayer2);
vidWin.open();

Note that in order to check all the constants you need to comment the line for any of the "modes" and uncomment another. The modes are matched by the corresponding number - having two players with the different constants should not show any difference.
Link for a sample video to download: https://download.blender.org/peach/bigbuckbunny_movies/

@build
Copy link
Contributor

build commented Aug 15, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 4336 tests are passing.
(There are 471 tests skipped)

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against 40f6016

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

@ypbnv, can you also update the "Media.yml" documentation please? We need to indicate that Android now supports these constants. Thanks!

warningMessage.append(" instead.");
Log.w("VideoPlayerProxy", warningMessage.toString());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The constantDeprecationWarning() method will end up appending strings every time the video view gets resized or a request-layout occurs. Even if you are using a non-deprecated constant. I'd prefer to avoid this. How about rewriting it to something like the below?

String message = null;
final String MESSAGE_FORMAT = "%s has been deprecated. Use %s instead.";
switch (constant) {
	case MediaModule.VIDEO_SCALING_ASPECT_FILL:
		message = String.format(MESSAGE_FORMAT, "Ti.Media.VIDEO_SCALING_ASPECT_FILL", "Ti.Media.VIDEO_SCALING_RESIZE_ASPECT_FILL");
		break;
	case MediaModule.VIDEO_SCALING_ASPECT_FIT:
		message = String.format(MESSAGE_FORMAT, "Ti.Media.VIDEO_SCALING_ASPECT_FIT", "Ti.Media.VIDEO_SCALING_RESIZE_ASPECT_FIT");
		break;
	case MediaModule.VIDEO_SCALING_MODE_FILL:
		message = String.format(MESSAGE_FORMAT, "Ti.Media.VIDEO_SCALING_MODE_FILL", "Ti.Media.VIDEO_SCALING_RESIZE");
		break;
}
if (message != null) {
	Log.w("VideoPlayerProxy", message);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's way better, yes. I also changed the default value of mScalingType to use the new constant counter part.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also changed the default value of mScalingType

I didn't even notice that. It's looking good now. :)

@build build added the docs label Aug 19, 2019
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@keerthi1032
Copy link
Contributor

FR Passed. New constants for video scaling behaves as same as old constants.
Test Environment:
Name = Mac OS X
Version = 10.14.5
Architecture = 64bit
Node.js
Node.js Version = 10.16.2
npm Version = 6.9.0
Titanium CLI
CLI Version = 5.2.1
Titanium SDK
SDK Version = local sdk 8.3.0.v20190909123424
Device -oneplus 5t android 9,samsung s5 android 6
Emulator = pixel 3xl android 9

@keerthi1032
Copy link
Contributor

android unit test is failing .not able to merge PR. @ypbnv can you please resolve Jenkins failure and merge PR

@keerthi1032 keerthi1032 merged commit 16f04c5 into tidev:master Sep 11, 2019
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