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.

Sign up for GitHub

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

Jump to bottom

[TIMOB-24665] Android: Fix constant value definitions #9031

Merged
merged 6 commits into from Jun 6, 2017
Merged

[TIMOB-24665] Android: Fix constant value definitions #9031

merged 6 commits into from Jun 6, 2017

Conversation

garymathews
Copy link
Contributor

  • Do not define constants on proxy instance
TEST CASE
Ti.API.info(
    'Ti.Media.AudioPlayer.STATE_BUFFERING: ' + Ti.Media.AudioPlayer.STATE_BUFFERING + '\n' +
    'Ti.Media.AudioPlayer.STATE_INITIALIZED: ' + Ti.Media.AudioPlayer.STATE_INITIALIZED + '\n' +
    'Ti.Media.AudioPlayer.STATE_PAUSED: ' + Ti.Media.AudioPlayer.STATE_PAUSED + '\n' +
    'Ti.Media.AudioPlayer.STATE_PLAYING: ' + Ti.Media.AudioPlayer.STATE_PLAYING + '\n' +
    'Ti.Media.AudioPlayer.STATE_STARTING: ' + Ti.Media.AudioPlayer.STATE_STARTING + '\n' +
    'Ti.Media.AudioPlayer.STATE_STOPPED: ' + Ti.Media.AudioPlayer.STATE_STOPPED + '\n' +
    'Ti.Media.AudioPlayer.STATE_STOPPING: ' + Ti.Media.AudioPlayer.STATE_STOPPING + '\n' +
    'Ti.Media.AudioPlayer.STATE_WAITING_FOR_DATA: ' + Ti.Media.AudioPlayer.STATE_WAITING_FOR_DATA + '\n' +
    'Ti.Media.AudioPlayer.STATE_WAITING_FOR_QUEUE: ' + Ti.Media.AudioPlayer.STATE_WAITING_FOR_QUEUE
);
 
Ti.API.info(
    'Ti.Media.Sound.STATE_BUFFERING: ' + Ti.Media.Sound.STATE_BUFFERING + '\n' +
    'Ti.Media.Sound.STATE_INITIALIZED: ' + Ti.Media.Sound.STATE_INITIALIZED + '\n' +
    'Ti.Media.Sound.STATE_PAUSED: ' + Ti.Media.Sound.STATE_PAUSED + '\n' +
    'Ti.Media.Sound.STATE_PLAYING: ' + Ti.Media.Sound.STATE_PLAYING + '\n' +
    'Ti.Media.Sound.STATE_STARTING: ' + Ti.Media.Sound.STATE_STARTING + '\n' +
    'Ti.Media.Sound.STATE_STOPPED: ' + Ti.Media.Sound.STATE_STOPPED + '\n' +
    'Ti.Media.Sound.STATE_STOPPING: ' + Ti.Media.Sound.STATE_STOPPING + '\n' +
    'Ti.Media.Sound.STATE_WAITING_FOR_DATA: ' + Ti.Media.Sound.STATE_WAITING_FOR_DATA + '\n' +
    'Ti.Media.Sound.STATE_WAITING_FOR_QUEUE: ' + Ti.Media.Sound.STATE_WAITING_FOR_QUEUE
);
  • Definitions should not be undefined

JIRA Ticket

@garymathews garymathews added this to the 6.2.0 milestone May 8, 2017
@garymathews garymathews requested a review from ypbnv May 8, 2017 14:27
ypbnv
ypbnv approved these changes May 9, 2017
View reviewed changes
Copy link
Contributor

@ypbnv ypbnv 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

@hansemannn
Copy link
Collaborator

@garymathews I saw your label BREAKING CHANGES. Should we move it to 7.0.0 then?

@garymathews
Copy link
Contributor Author

@hansemannn I'm not sure, the reason it's a breaking change is due to this:

var audioPlayer = Ti.Media.createAudioPlayer(...);
audioPlayer.STATE_BUFFERING // this will now be undefined

Ti.Media.AudioPlayer.STATE_BUFFERING // this should be used instead

But Ti.Media.AudioPlayer.STATE_BUFFERING is the documented way, I'm just concerned about user's using a workaround.

@hansemannn
Copy link
Collaborator

Can we do both and deprecate the instance constant? I'm afraid any breaking change would need to wait until 7.0.0 :(.

@sgtcoolguy
Copy link
Contributor

Can we also add unit tests in this PR to our test suite? I don't think we have any for Ti.Media.*, so adding tests that just verify these contents would be awesome.

(To add the tests, drop in a new ti.media.audioplayer.test.js and ti.media.sound.test.js into tests/Resources, then copy https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/app.js to tests/Resources/app.js and add requires for them.

@sgtcoolguy sgtcoolguy self-requested a review May 11, 2017 15:42
sgtcoolguy
sgtcoolguy suggested changes May 11, 2017
View reviewed changes
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

Add unit tests to verify the constants live at the right level.

This would be a good place to crib some constant tests: https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/1102d6e1d7a0da2bb41082b350a6f22d08f449fa/Resources/ti.geolocation.test.js

@garymathews
Copy link
Contributor Author

@sgtcoolguy Updated PR, added test cases.

@garymathews garymathews mentioned this pull request Jun 5, 2017
Merged
@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Jun 5, 2017 •
edited

FR Passed.

Ran the above code & the definitions does not return undefined.
Output:
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_BUFFERING: 0
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_INITIALIZED: 1
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_PAUSED: 2
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_PLAYING: 3
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_STARTING: 4
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_STOPPED: 5
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_STOPPING: 6
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_WAITING_FOR_DATA: 7
[INFO] : [Pixel] Ti.Media.AudioPlayer.STATE_WAITING_FOR_QUEUE: 8
[INFO] : [Pixel] Ti.Media.Sound.STATE_BUFFERING: 0
[INFO] : [Pixel] Ti.Media.Sound.STATE_INITIALIZED: 1
[INFO] : [Pixel] Ti.Media.Sound.STATE_PAUSED: 2
[INFO] : [Pixel] Ti.Media.Sound.STATE_PLAYING: 3
[INFO] : [Pixel] Ti.Media.Sound.STATE_STARTING: 4
[INFO] : [Pixel] Ti.Media.Sound.STATE_STOPPED: 5
[INFO] : [Pixel] Ti.Media.Sound.STATE_STOPPING: 6
[INFO] : [Pixel] Ti.Media.Sound.STATE_WAITING_FOR_DATA: 7
[INFO] : [Pixel] Ti.Media.Sound.STATE_WAITING_FOR_QUEUE: 8

[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_BUFFERING: 0
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_INITIALIZED: 1
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_PAUSED: 2
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_PLAYING: 3
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_STARTING: 4
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_STOPPED: 5
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_STOPPING: 6
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_WAITING_FOR_DATA: 7
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.AudioPlayer.STATE_WAITING_FOR_QUEUE: 8
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_BUFFERING: 0
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_INITIALIZED: 1
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_PAUSED: 2
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_PLAYING: 3
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_STARTING: 4
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_STOPPED: 5
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_STOPPING: 6
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_WAITING_FOR_DATA: 7
[INFO] : [SAMSUNG-SGH-I337] Ti.Media.Sound.STATE_WAITING_FOR_QUEUE: 8

Studio Ver: 4.9.0.201705302345
SDK Ver: 6.2.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.2
Appc NPM: 4.2.9
Appc CLI: 6.2.2
Ti CLI Ver: 5.0.14
Alloy Ver: 1.9.11
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Devices: ⇨ samsung SAMSUNG-SGH-I337 --- Android 4.4.4
⇨ google Pixel --- Android 7.1.1

@mukherjee2 mukherjee2 merged commit 1c2d341 into tidev:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgtcoolguy sgtcoolguy sgtcoolguy requested changes

@ypbnv ypbnv ypbnv approved these changes

Assignees
No one assigned
Labels
android bug in-qe-testing 🕵
Projects
None yet
Milestone
6.2.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@garymathews @hansemannn @sgtcoolguy @lokeshchdhry @ypbnv @mukherjee2