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

fileName parameter on iOS does not work the way it is documented? #48

Open
spacepope opened this issue Jun 19, 2018 · 7 comments
Open
Labels

Comments

@spacepope
Copy link

Hi @JoschkaSchulz ,

i just noticed on iOS the resized image file url returned from the plugin is always an auto generated filename (img%d.jpg with %d being a static count variable, line 124 in ImageResizer.m).
Looking into the code, it does seem like fileName parameter on iOS is used as an asset name which can be used instead of the uri parameter to load the source image.. (lines 58 et sec., correct me if i am wrong..)
If this is the way it works on iOS - why fileName is mandatory like stated in #16 ?

Different behaviour on different platforms is not the way a cordova plugin should be implemented and different parameter meanings on different platforms is really bad practice (i would actually say a no go).
Even worse, the documentation does not mention this. Doc says: A custom name for the file. Default name is a timestamp. , which is absolutely not true for iOS.

I would recommend to at least clarify the meaning of fileName parameter in the docs with a big hint on different behavior. Better: implement a new optional parameter like assetIdentifier for this iOS specific stuff and change the filename generation to work like in Android..

@salzig
Copy link

salzig commented Jun 20, 2018

Feel free to provide a {merge,pull} request to change the behaviour :)

@JoschkaSchulz
Copy link
Owner

Thank you for your feedback. But if you already looked inside the files it would be the best if you would create a pull request (like salzig said). So It could be better for every next one to step inside the wrong readme.

@spacepope
Copy link
Author

As you probably noticed my issue title is expressed as a question. Although i looked inside the code for 5 minutes to see why i am getting unexcpected results, i only make assumptions on how the code could be meant to work.
Changing the behaviour of iOS part could potentially be a breaking change (maybe people already rely on getting this file format on iOS or on using asset names via the fileName parameter..).
So it absolutely makes sense to discuss such things before providing PRs.

Besides: you didn't at least answer one of my questions.. 😉

@JoschkaSchulz
Copy link
Owner

@spacepope, I looked inside the code because I knew it was working in the past so I wanted to know what happens... Like I mentioned in other issues already this whole repo isn't tested and I don't want to invest time to set up everything (I'm still wondering why suddenly so many people start using this thing, it was written for a special case for a german company, now it seems to get used by far more Oo)

The branch that introduced the name was this one: 1a1d808

the branch that broke this was this one: c958c05

No wonder why there is confusion about that... but in the last 7 month no one points that out the most people seem to work on android and ignored that the fileName wasn't working correctly on iOS

@spacepope
Copy link
Author

spacepope commented Jun 22, 2018

I'm still wondering why suddenly so many people start using this thing

That's funny - because your repo is officially listed in the Ionic frameworks plugins 😄 Congrats 🎉 😉

If i got some free time some day, i could do a PR with updated docs.

Just to be sure: is my assumption regarding those asset part on iOS (fileName parameter is used to load an asset instead of the uri parameter) correct?
Would be great if you could clarify the way you wanted this to work, so that anyone who wants to work on this knows.. (otherwise someone could get the idea to simply remove this asset loading part..)

@JoschkaSchulz
Copy link
Owner

@spacepope yeah but I have no idea why it's listen there... I havn't added it there and nobody asks me about the status of this repo :/

@muhammadomais
Copy link

Is this issue of the IOS fixed? I am still getting the error.

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

No branches or pull requests

4 participants