-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: DATA_URL is improperly prefixed #432
Conversation
fd45329
to
b849328
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the snake_case style in Android, otherwise LGTM
I will create a new PR to remove all old snake_case in the Android implementation to be compliant with Java Standards. |
@@ -1,311 +0,0 @@ | |||
/* global Q, resolveLocalFileSystemURL, Camera, cordova */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same. CI is green though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appium tests are currently removed from the project, as paramedic can't properly run them. PR #469 adds them back when the problems are fixed. But I am confused why this PR removes that file - because it should already be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually weird because this file was removed from master as a certain point in time. I will re-sync with master again and resolve any merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking it out, I think appium-tests/helpers/cameraHelper.js is removed in this commit:
19d8e2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR had some updates to this cameraHelper.js
file in order to assert that DATA_URL is properly prefixed, so not having these unit tests can cause issues while trying to fix #469 (Because the old unit tests were asserting the wrong DATA_URLs :)). So here is my proposed plan:
- Postponing my PR fix: DATA_URL is improperly prefixed #432 until fix: add back Appium tests #469 is resolved.
- Once PR fix: add back Appium tests #469 is resolved, I will sync-up and add the relevant unit tests for the DATA_URL feature.
- Once finished, PR fix: DATA_URL is improperly prefixed #432 will be re-pushed.
Does this sound a good plan?
Platforms affected
All (Android, iOS, OS X, Windows and Browser)
Motivation and Context
When calling
navigator.camera.getPicture
withdestinationType
set toCamera.DestinationType.DATA_URL
, the success function is called with the base64 encoded data while it should has suitable data URL format on the following form:data:[<mediatype>][;base64],<data>
It is an old issue in the library and it was previously reported under:
https://issues.apache.org/jira/browse/CB-9819
And recently, it was also reported by one of the library users:
#420
Description
I have implemented and tested the DATA_URL fix across all of the Camera plugin supported platforms:
Testing
I manually tested this change across all of these platforms and they work fine.
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)