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

fix(android)(9_3_X): AudioRecorder recording/stopped property handling #11996

Merged
merged 8 commits into from Oct 15, 2020

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Sep 1, 2020

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

Summary:

  • AudioRecoder changes:
    • Fixed bug where it reports the wrongs states via recording and stopped properties
    • Now auto-stops when JS object is garbage collected or JS runtime is terminated.
    • Removed blocking calls from start() and resume() methods. (Can block up to 10 sec.)
    • Now checks if recording has actually started. (Can't start if microphone is already in use.)
    • Significantly optimized stop() method. (No longer creates 2nd audio file.)
  • Added a new "build.post.install" build hook for Android and iOS.
  • Added AudioRecorder unit tests for Android and iOS.
  • Updated test suite's plugin to grant app permissions to iOS simulator and Android after app install.

Test:

  1. Create a "Classic" app.
  2. Add the below permission to the "tiapp.xml".
  3. Use AudioRecorderStatusTest.js attached to TIMOB-28105 for the "app.js".
  4. Build and run on Android.
  5. Verify "Stopped" label is true and the other labels are false.
  6. Tap on the "Start Recording" button.
  7. Verify "Recording" label is true and the other labels are false.
  8. Tap on the "Pause Recording" button.
  9. Verify "Paused" label is true and the other labels are false.
  10. Tap on the "Resume Recording" button.
  11. Verify "Recording" label is true and the other labels are false.
  12. Tap on the "Stop Recording" button.
  13. Verify "Stopped" label is true and the other labels are false.

tiapp.xml

<ti:app>
	<android>
		<manifest>
			<uses-permission android:name="android.permission.RECORD_AUDIO"/>
		</manifest>
	</android>
</ti:app>

@build
Copy link
Contributor

build commented Sep 2, 2020

Fails
🚫 Tests have failed, see below for more information.
Warnings
⚠️

Commit 9b887eb24fb07b7fb8efb55e354f7de82ddfb853 has a message "fix(android): AudioRecorder recording/stopped property handling

Fixes TIMOB-28105" giving 1 errors:

  • subject must not be sentence-case, start-case, pascal-case, upper-case
⚠️

build/lib/test/test.js#L443 - build/lib/test/test.js line 443 – 'stripped' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

build/lib/test/test.js#L675 - build/lib/test/test.js line 675 – Found child_process.exec() with non Literal first argument (security/detect-child-process)

⚠️

build/lib/test/test.js#L779 - build/lib/test/test.js line 779 – Missing JSDoc parameter description for 'testResults'. (valid-jsdoc)

⚠️

tests/Resources/app.js#L12 - tests/Resources/app.js line 12 – 'OS_MACOS' is assigned a value but never used. (no-unused-vars)

Messages
📖

💾 Here's the generated SDK zipfile.

📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

📖 ❌ 14 tests have failed There are 14 tests failing and 976 skipped out of 10824 total tests.

Tests:

ClassnameNameTimeError
ios.macos.Titanium.UI.iOS.CollisionBehavior.exampleworks (10.15.5)15.03
Error: timeout of 15000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Window.hidesBackButton (10.15.5)20.027
Error: timeout of 20000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.TabGroupadd Map.View to TabGroup (10.15.5)15.046
Error: timeout of 15000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4290:32
ios.macos.Titanium.UI.TableViewrow#rect (10.15.5)5.069
Error: timeout of 5000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.TextAreatextArea in tabGroup (10.15.5)7.502
Error: timeout of 7500ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4290:32
ios.macos.Titanium.UI.Viewanimate (height %) (10.15.5)10
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (width %) (10.15.5)10.058
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (top %) (10.15.5)10.022
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (left %) (10.15.5)10.019
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (left) (10.15.5)10.101
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (top) - autoreverse (10.15.5)10.102
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.Viewanimate (top) (10.15.5)10.09
Error: timeout of 10000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4326:27
ios.macos.Titanium.UI.View.borderRadius corners1 value with shadow effect (10.15.5)0.087
Error: expected 'Ti.UI.View' view to match snapshot image: snapshots/borderRadiusWithShadow30px.png
fail@file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/node_modules/should/cjs/should.js:275:23
value@file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/node_modules/should/cjs/should.js:356:23
postlayout@file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti.ui.view.test.js:1212:39
ios.macos.Titanium.Utils#base64encode() Ti.Blob#TYPE_DATA from Ti.UI.View.toImage() async (10.15.5)5.087
Error: timeout of 5000ms exceeded
file:///Users/build/jenkins/workspace/ium-sdk_titanium_mobile_PR-11996/tmp/mocha/build/iphone/build/Products/Debug-maccatalyst/mocha.app/Contents/Resources/ti-mocha.js:4290:32

Generated by 🚫 dangerJS against 79f5111

@jquick-axway jquick-axway changed the title fix(android): AudioRecorder recording/stopped property handling fix(android)(9_3_X): AudioRecorder recording/stopped property handling Sep 2, 2020
out.close();
} catch (IOException e) {
e.printStackTrace();
if (this.audioRecord != null) {
Copy link
Contributor

@sgtcoolguy sgtcoolguy Sep 4, 2020

Choose a reason for hiding this comment

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

Same as above, do we need to guard consecutive resume calls? (or some other state?) I think the null check guards against resuming after stopped or before started.

@@ -90,8 +90,11 @@ exports.init = function (logger, config, cli) {
logger.error('[' + simHandle.appName + '] ' + msg);
})
.on('app-started', function () {
finished && finished();
finished = null;
// TODO: Add "installed" event to "ioslib" module for simulators and emit below event from there.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fairly straightforward to add here: https://github.com/appcelerator/ioslib/blob/1_7_X/lib/simulator.js#L1586

Basically instead of passing next to simctl.install as the callback, add a function that emits the event (if no error) and then fires next().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about making changes to ioslib, but I saw that major changes are being made to it on master. And I have no idea what's going on with the 2_X and 3_X branches. Are they used by the daemon and Appc Studio?

await xcrun([ 'simctl', 'privacy', builder.simHandle.udid, 'grant', 'all', builder.tiapp.id ]);

// Re-launch app in case granting permissions forced-quit it.
await xcrun([ 'simctl', 'launch', builder.simHandle.udid, builder.tiapp.id ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is because of the note above where we're not actually firing post-install, pre-launch - we're firing post-launch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly right. We'd be able to remove this line if we could fire the post install event before the launch.

Copy link
Contributor

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

@jquick-axway jquick-axway added the backport master when applied, PRs with this label will get an auto-generated backport to master branch on merge label Sep 21, 2020
@ssekhri
Copy link

ssekhri commented Oct 14, 2020

FR Passed.
Verified on:
Mac OS: 10.15.4
SDK: 9.3.0.v20201005141423
Appc CLI: 8.1.1
JDK: 11.0.4
Node: 10.17.0
Studio: 6.0.0.202005141803
Device: Pixel 3(v11.0) device, Pixel 3 XL(v11.0) emulator

- Now auto-stops recording when JS object is released or JS runtime has been terminated.
  * Needed to to free up microphone to be used by next audio recorder object.
- Modified AudioRecorder.start() to no-op if already started.
- Removed blocking reads from audio recorder. Now always reads microphone from worker thread.
- Now checks if audio recording was successfully started.
@sgtcoolguy sgtcoolguy merged commit b25ee6c into tidev:9_3_X Oct 15, 2020
@build
Copy link
Contributor

build commented Oct 15, 2020

The backport to master failed:

The process 'git' failed with exit code 128

Check the run for full details
To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Check out the target branch
git checkout master
# Make sure it's up to date
git pull
# Check out your branch
git checkout -b backport-11996-to-master
# Apply the commits from the PR
curl -s https://github.com/appcelerator/titanium_mobile/commit/aa2d1ba047198f0624c2769a18dee70c00e5cb72.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/22e8cc0a1e23487f82f9a039899214640834eaa0.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/29277557f1fe68b49b39595f538ffe2a3629cad7.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/8335e27ec6db2d3758bd637121973b430166ce94.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/6c3869af9044ff9a107a4b8c692dbaa3cbcf5e57.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/68a1057a2b897ccac65045fa1b655a7570376d9f.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/487c403502fad18c7e6e531c56ce6c9f74dea796.patch | git am -3 --ignore-whitespace
curl -s https://github.com/appcelerator/titanium_mobile/commit/397cbb47ffde0b6a7e2ff97b725e0b5ecefbcf62.patch | git am -3 --ignore-whitespace
# Push it to GitHub
git push --set-upstream origin backport-11996-to-master

Then, create a pull request where the base branch is master and the compare/head branch is backport-11996-to-master.

@ewanharris ewanharris removed backport master when applied, PRs with this label will get an auto-generated backport to master branch on merge master backport failed labels Mar 16, 2021
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

6 participants