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

Provide success and error callbacks, and the "Not an image" error. #48

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Hypher
Copy link

@Hypher Hypher commented Dec 8, 2016

Moved callbacks into the async task on iOS

No API changes, just two optional parameters

@sarriaroman
Copy link
Owner

Are you able to get the changes on Android too?

@Hypher
Copy link
Author

Hypher commented Dec 23, 2016

Android is already playing nicely showing a toast into Picasso when it's not an image.
And the "success and error callbacks" part works for both android and ios.
So I think we can say android does not really need more changes

@BramDecuypere
Copy link

I disagree with you @Hypher. Because what if on android we have already denied the permissions and never ask again? There is no way to provide feedback!

@Hypher
Copy link
Author

Hypher commented Jan 2, 2017

This issue is not about permission. It's about:

  • fixing a CRASH on iOS when it's not an image
  • providing callbacks MAINLY to report that error to JS level

Yes it's not consistent with Android since Android does not send the very same message through the bridge when its "not an image" ; instead it shows a toast in Picasso, and report an error through the bridge. But it was not consistent too when iOS crashed... (and when there was just NO callbacks at all)

So, this pull request fixes an important issue, and provide more feedback, and must be accepted.

Then, if you want, you can make your API consistent, and maybe deal with permissions etc.
But that's another problems.

@BramDecuypere
Copy link

Hi Hypher? So calling the code without having the right permissions won't result in an error? There will be no way to find this out?

@Hypher
Copy link
Author

Hypher commented Jan 3, 2017

If there is no permission it will call the error callback with :
this.callbackContext.sendPluginResult(new PluginResult(PluginResult.Status.ERROR, PERMISSION_DENIED_ERROR));
But this is out of the scope of this request.

@sarriaroman sorry but I modified the package name in the last two commits to be able to use it in my project. Feel free to drop these changes when you merge it

@sarriaroman
Copy link
Owner

I will need to get the changes for Android before merge it. The plugin is intended for both.

@WuglyakBolgoink
Copy link

WuglyakBolgoink commented Feb 17, 2017

@Hypher thanks for PR (see my fork only for iOS: https://github.com/WuglyakBolgoink/CkOpenImage)

@sirineKr
Copy link

Could you merge this PR please. I use this plugin to show photo and i need callback function to define actions when user click on close button.
thank you in advance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants