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): handle raw document identifier #11395

Merged
merged 6 commits into from Jan 9, 2020

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Dec 13, 2019

  • Implement handling for raw document identifier
  • Prevent unhandled exception
TEST CASE
const win = Ti.UI.createWindow({ backgroundColor: 'white' });
const img = Ti.UI.createImageView();

win.addEventListener('open', _ => {
    Ti.Media.openPhotoGallery({
        success (event) {
            if (event.mediaType == Ti.Media.MEDIA_TYPE_PHOTO) {
                img.image = event.media;
            }
        },
        mediaTypes:[Ti.Media.MEDIA_TYPE_PHOTO]
    }
});

win.add(img);
win.open();
  • Select photo from Downloads

JIRA Ticket

@build
Copy link
Contributor

build commented Dec 13, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

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

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

Generated by 🚫 dangerJS against f3c78fd

@sgtcoolguy
Copy link
Contributor

@garymathews Does this PR supersede the community PR #11223?

@garymathews
Copy link
Contributor Author

@sgtcoolguy Yep

@sgtcoolguy sgtcoolguy mentioned this pull request Dec 18, 2019
@sgtcoolguy
Copy link
Contributor

Can we add an Android-specific unit test for this? It's hard to trace this back to see if there's a clear cut API we can use to trigger this code path, but maybe Ti.Filesystem.getFile('content://com.android.providers.downloads.documents/document/raw%3A%2Fstorage%2Femulated%2F0%2FDownload%2Fa.pdf')

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.

code change looks OK to me, but as stated in other comment - I'd really like to see if there's any way we can test this with a URL passed in to one of our APIs...

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

Although I kind of want to remove all of this custom URL handling code and handle this 100% via a ContentResolver which is what we do at the bottom of this method. That's the way you're supposed to handle content:// URLs on Android. But this would mean re-testing all of the URLs we're custom handling just in case.

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Able to select content from downloads folder without the application crashing.

Test Steps

  1. Created a Titanium application with the following test case
const win = Ti.UI.createWindow({ backgroundColor: 'white' });
const img = Ti.UI.createImageView();

win.addEventListener('open', _ => {
    Ti.Media.openPhotoGallery({
        success (event) {
            if (event.mediaType == Ti.Media.MEDIA_TYPE_PHOTO) {
                img.image = event.media;
            }
        },
        mediaTypes:[Ti.Media.MEDIA_TYPE_PHOTO]
    })
});

win.add(img);
win.open();
  1. Ran the application
  2. Selected downloads
  3. Selected Image
  4. Image displayed without the application crashing

Test Environment

MacOS Catalina 10.15.1 beta
Xcode 11.3
Node.js 10.16.3
"NPM":"4.2.15-1","CLI":"7.1.2-7"
Pixel Xl (Android 10)

@sgtcoolguy sgtcoolguy merged commit c2d89d4 into tidev:master Jan 9, 2020
@ygbr
Copy link

ygbr commented Feb 17, 2020

can we get this fix backported to 8_3_X ? Seems simple enough to apply on https://github.com/appcelerator/titanium_mobile/blob/e9ef3702af3f75327af9056ec0bb9a7bf209e130/android/titanium/src/java/org/appcelerator/titanium/io/TitaniumBlob.java#L89 and would allow 8.3.X to get android file delete working again...

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