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

refactor(android): media/file APIs to use scoped storage #12143

Merged
merged 16 commits into from Oct 13, 2020

Conversation

jquick-axway
Copy link
Contributor

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

JIRA:

Summary:

  • TIMOB-28059 Modified Ti.Media.showCamera() and saveToPhotoGallery() to use scoped storage.
    • No longer requires external storage permission on Android 10 and higher.
  • Camera Overlay Changes:
    • TIMOB-27481 Fixed bug where navigating back would may wrongly exit out of the app.
    • TIMOB-26602 Fixed bug where Ti.Media.takePicture() may use the wrong file extension.
  • Fixed Ti.Media.VideoPlayer crash which can occur if error callback is invoked during playback.
  • TIMOB-28183 Added media methods:
    • Ti.Media.hasPhotoGalleryPermissions()
    • Ti.Media.requestPhotoGalleryPermissions()
  • TIMOB-27201 Updated Ti.Filesystem.File APIs when wrapping "content://" URL.
    • Added: createdAt(), deleteFile(), exists(), extension(), modifiedAt(), read(), readonly, write(), writable
    • Missing: createDirectory(), open(), readLine()
  • Optimized Ti.Filesystem.File object's move() method when performed on same volume.
  • Updated Ti.Filesystem.File object's rename() to support absolute path in same folder.
  • Renamed TitaniumBlob.java to TiContentFile.java.
    • New name better indicates it supports "content://" URLs.
  • Removed unused "ti.android.enablecoverage" related code. (Was never supported.)

Camera Photo Test:
(Run this test on Android 11 and Android 4.4.)

  1. Create a Classic Titanium app with the below "tiapp.xml" settings.
  2. Build and run "CameraPhotoExternalTest.js" attached to TIMOB-28059.
  3. Tap on the "Take Picture" button.
  4. On Android 6.0 and higher, verify app requests permission. Agree to it.
  5. Verify a camera window appears.
  6. Tap the Back button to go back to the app. Verify app does not exit.
  7. Tap on the "Take Picture" button.
  8. Take a picture and hit okay in the camera window.
  9. Verify the photo you taken is displayed by the Titanium app.
  10. In the JS code, uncomment line: // saveToPhotoGallery: true
  11. Repeat steps 3-9.
  12. Go to the Photos/Gallery app and verify you can find your photo there.
  13. Build and run "CameraPhotoInternalTest.js" attached to TIMOB-28059.
  14. Repeat steps 3-12.
<ti:app>
	<android>
		<manifest>
			<uses-sdk android:targetSdkVersion="30"/>
			<uses-permission android:name="android.permission.CAMERA"/>
		</manifest>
	</android>
</ti:app>

Camera Video Test:
(Run this test on Android 11 and Android 4.4.)

  1. Create a Classic Titanium app with the below "tiapp.xml" settings.
  2. Build and run "CameraVideoExternalTest.js" attached to TIMOB-28059.
  3. Tap on the "Capture Video" button.
  4. On Android 6.0 and higher, verify app requests permission. Agree to it.
  5. Verify a camera window appears.
  6. Tap the Back button to go back to the app. Verify app does not exit.
  7. Tap on the "Capture Video" button.
  8. Record video and hit okay in the camera window.
  9. Verify the Titanium app displays and plays your video.
  10. In the JS code, uncomment line: // saveToPhotoGallery: true
  11. Repeat steps 3-9.
  12. Go to the Photos/Gallery app and verify you can find your video there.
  13. Build and run "CameraVideoInternalTest.js" attached to TIMOB-28059.
  14. Repeat steps 3-12.
<ti:app>
	<android>
		<manifest>
			<uses-sdk android:targetSdkVersion="30"/>
			<uses-permission android:name="android.permission.CAMERA"/>
			<uses-permission android:name="android.permission.RECORD_AUDIO"/>
		</manifest>
	</android>
</ti:app>

Save Image File to Gallery Test:
(Run this test on Android 11 and Android 4.4.)

  1. Create a Classic Titanium app with the below "tiapp.xml" setting.
  2. Build and run "ImageToGalleryTest.js" attached to TIMOB-28059.
  3. Tap on the "Save Screenshot to Gallery" button.
  4. Verify an alert displays it was successful.
  5. Open the Photos/Gallery app and verify you can find the screenshot there.
<ti:app>
	<android>
		<manifest>
			<uses-sdk android:targetSdkVersion="30"/>
		</manifest>
	</android>
</ti:app>

- TIMOB-27201: Updated Ti.Filesystem.File to support below APIs when wrapping "content://" URL:
  * createdAt(), deleteFile(), exists(), modifiedAt(), read(), readonly, write(), writable
- Updated Ti.Filesystem.File to support below APIs when resource files:
  * createdAt(). modifiedAt()
- Optimized Ti.Filesystem.File move() when performed on the same volume.
- TIMOB-28146: Updated Ti.Filesystem.File rename() to support absolute path in same directory and source.

Fixes TIMOB-27201, TIMOB-28146
- TIMOB-28059: Modified Ti.Media.showCamera() and saveToPhotoGallery() to use scoped storage.
  * Removed requirement for WRITE_EXTERNAL_STORAGE and READ_EXTERNAL_STORAGE permissions.
- TIMOB-27481: Fixed bug where navigating back from camera overlay may wrongly close the app.
- TIMOB-26602: Fixed bug where Ti.Media.takePicture() may use wrong file extension.
- Fixed Ti.Media.VideoPlayer crash that can occur if error callback is invoked during playback.

Fixes TIMOB-28059, TIMOB-27481, TIMOB-26602
@build
Copy link
Contributor

build commented Oct 1, 2020

Warnings
⚠️

Commit f6bb5b1da38151f65a2ffbcbb5a9eb7c5e9e89e2 has a message "refactor(android): content url and resource file handling

  • TIMOB-27201: Updated Ti.Filesystem.File to support below APIs when wrapping "content://" URL:
    • createdAt(), deleteFile(), exists(), modifiedAt(), read(), readonly, write(), writable
  • Updated Ti.Filesystem.File to support below APIs when resource files:
    • createdAt(). modifiedAt()
  • Optimized Ti.Filesystem.File move() when performed on the same volume.
  • TIMOB-28146: Updated Ti.Filesystem.File rename() to support absolute path in same directory and source.

Fixes TIMOB-27201, TIMOB-28146" giving 1 errors:

  • body's lines must not be longer than 100 characters
⚠️

Commit 89c5a5f05b009feee546378904fc67fc1ca2ec04 has a message "Merged" giving 2 errors:

  • subject may not be empty
  • type may not be empty
⚠️

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

⚠️

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

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.

📖

✅ All tests are passing
Nice one! All 9908 tests are passing.
(There are 983 skipped tests not included in that total)

Generated by 🚫 dangerJS against 137838d

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

Finally got rid of TitaniumBlob.java, much better named now 😃

});

it.androidAndIosBroken('file:// relative path', () => {
// FIXME: Android seems to basically forcibly place '/' in front of paths not beginning with '..' or '/'
// FIXME: iOS does not seem to try and resolve relative paths for file:// URIs
const file = Ti.Filesystem.getFile('file://app.js'); // app.js should be relative to this file...
should.exist(file);
should(file.exists()).equal(true);
should(file.exists()).be.true();
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: Should these tests be enabled on Android now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test still won't work on Android. We would have to modify TiFileFactory to check if the "file://" URL has a relative and have it use our TiResourceFile instead of a TiFile object since our APK's "assets/Resources" folder is not accessible from the file system.

But the problem here is if you set a relative path without a URL scheme like this...

Ti.Filesystem.getFile('app.js');

...then TiFileFactory defaults to the applicationDataDirectory, not resourcesDirectory like this unit test assumes. So, we have an inconsistency here.

@build build added the docs label Oct 10, 2020
@ssaddique
Copy link
Contributor

ssaddique commented Oct 13, 2020

FR: Pass

Test Environment
SDK Ver: 9.2.1.GA, this PR
OS Ver: 10.15.6
Appc NPM: 5.0.0
Appc CLI: 8.1.1
Node Ver: 10.17.0
NPM Ver: 6.13.6
Java Ver: 1.8.0_162
Devices: Pixel 3a (11.0)
Emulator: Pixel 2 (4.4 - 8.1), Pixel 3 (9.0 - 10.0), Pixel 4 (11.0)

@ssaddique ssaddique merged commit 9b42601 into tidev:master Oct 13, 2020
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

4 participants