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

Replaced ELCImagePicker => GMImagePicker #84

Closed
wants to merge 34 commits into from

Conversation

hanssens
Copy link

In short, this is a severe refactoring which basically replaces the ELCImagePicker with the GMImagePicker in order to support lacking iOS 8 features with the first.

Effort has been put into this by @MrBasque, @micheladrion and @hanssens.

Features & Improvements

  • Supports iOS 8 Photos Framework, incl. shared albums and non-local photos (optimized cloud storage)
  • [IOS] Fixes issue Selecting photos from a Shared Album results in 'null' #56: Selecting photos from a Shared Album results in 'null'
  • [IOS] Fixes issue Unselectable photos and empty albums when Optimized Storage is enabled #57: Unselectable photos and empty albums when Optimized Storage is enabled
  • [IOS] Enables fetching of images on-demand that aren't stored locally
  • [IOS+Android] Response now returns both original image path but now also the thumb path as well
  • No further clientside/consumer impact
  • [IOS] Added orientation-fix to correctly display the returned result
  • [Enhancement] Options allow you to enable or disable selection of video's through allow_video
  • [Enhancement] Options allow you to provide a custom title and message in the selection screen

Important Changes

  • Removed ELCImagePicker in favor of GMImagePicker
  • [IOS] Changed DeliveryMode from "FastFormat" => "HighQualityFormat" in GMImagePicker
  • [IOS] Only support for iOS 8+

Known issues

  • Fetching duration of an image from iCloud (on-demand) can range from a second to a couple of seconds
  • Could seriously use a codereview

Jonathan Aquino and others added 27 commits October 29, 2014 14:29
I got rid of the lines between the albums. Also I put some padding around the
album thumbnails.

See this screenshot for the improvements:
http://monosnap.com/image/wcBd9YW65Y0AWFAVhUy0xgLoUx0pV5

I have created a similar pull request for the original ELCImagePickerController
project: B-Sides/ELCImagePickerController#98
…(ex: 1500x1001 pixel image gets a white line on the bottom)
…entered, log to the console and default to FILE_URI
Replaced ELCImagePicker => GMImagePicker
@EddyVerbruggen
Copy link

Awesome job - I'm seriously considering reviewing/testing/adding this to my fork. Perhaps it needs some kind of graceful iOS7 handling but need to study this first a bit.

- New option "allow_video": which enables or disables selection of video's
- New option "title": provide a custom title in the header of the selection screen
- New option "message": provide a custom subtitle in the header of the selection screen
@hanssens
Copy link
Author

In the latest commit the plugin.xml is now included as well. This means it's quite final and ready for comments. Despite that we're all quite busy and this PR is a tough one to swallow, I would sure like to hear if this something that's worth continuing on. Thanks!

- Updated header and source files to reflect GMImagePicker
- Suggested version increment to to v2.0.0
@danieltigse
Copy link

Great job!! Please release these changes. I need it for my work!

@hanssens
Copy link
Author

@TheKiteEatingTree + @CSullivan102 perhaps you might want to share your quick thoughts on this PR?

@hanssens
Copy link
Author

For those interested, this update works just fine on iOS 9 as well.

@hanssens
Copy link
Author

Note that since the last commit to master, about 8 days ago, there seem to be conflicts. This is logical, considering this PR totally replaces ELC.

But if there's any interest, I'll gladly update it to the latest master. /cc @ DMcNamara

@uxtx
Copy link

uxtx commented Oct 23, 2015

Definitely interested in this PR. I'm running into issues with users trying to select images from icloud... it sure sounds like this could help with that.

@hanssens
Copy link
Author

Awesome, thanks. This indeed is iCloud-compatible and works great in production. Although, as @EddyVerbruggen also mentioned, it breaks iOS 7 support and should perhaps be handled accordingly.

With this in mind, it would justify making a v2.0 release perhaps?

@nod
Copy link

nod commented Oct 28, 2015

Hi - we, too, are having issues with icloud. Any update on this PR?

@EddyVerbruggen
Copy link

Hey @hanssens, I've reviewed you changes because I felt it was time folks shouldn't care about iOS7 too much anymore ;)

I've pulled your cool PR to the fork I maintain here and made a few changes on top (these are partly personal taste, so don't take it ehm.. personally):

  • Changed the default title and message so they reflect how it looked in the previous version of the plugin (no nasty surprises for developers).
  • Re-added base64 encoding of iOS images which was already available in my fork.
  • I removed the thumbnail feature to remain compatible with users of the old version (this also gets rid of two new jars Android needed).
  • I've re-added the width, height and quality params which were still used for Android, but no longer for iOS after this PR was merged.
  • Fixed an (old) issue on Android where base64 encoded images didn't obey the passed-in quality setting.
  • Removed the 'fetching' text on top of the progress indicator on iOS. I thought it was a bit too much ;)

@hanssens
Copy link
Author

@EddyVerbruggen I can only say... excellent! 👍

Synching repo with 'Telerik-Verified-Plugins' fork
@DMcNamara
Copy link
Contributor

Hey, just wanted to let you all know that we haven't forgotten about this and aren't just ignoring it. This PR is awesome, and I'm really excited about it. It's also really big. We're a small team and always busy (who isn't, though?), but I've been trying to steal more time to maintain our OS projects whenever I can. This PR is top on my list, and I am actively reviewing it whenever I get a chance, so hopefully we'll have a 2.0.0 release soon!

@hanssens
Copy link
Author

hanssens commented Nov 4, 2015

Thanks for the follow-up, @DMcNamara. Sounds great! Let me know where I can assist...

@DMcNamara
Copy link
Contributor

I spent a long train ride last weekend going through this and it looks really great. I've just got a few comments and questions on mainly documentation stuff.

  • allow_video should really be camelCase allowVideo, at least in imagepicker.js, since the maximumImagesCount option is already camelCase. It's easier if option naming is consistent.
  • In the readme:
    • Do any of the examples need to be updated? I don't think so, but I'm not sure.
    • Are there any new features that should be mentioned in there? I see the new outputType option listed in the options, but it would be good to list the other new options, like allow_video, title, and message too. There's probably some other things that could use a mention too.
    • Something about upgrading from 1.0 would also probably be good. Definitely need to mention dropping support for iOS 7.
    • There's also a paragraph at the end that mentions ELCImagePicker that should now say GMImagePicker.
  • There's a // TODO pass in quality comment in GMGridViewController.m. It seems like it does quality conversion elsewhere, so is that just for display, or is it a forgotten comment?
  • Having a demo is awesome, but I think there's one comment in there that should probably be removed before release since it seems development related, the one about "if no title is passed".

@bagusflyer
Copy link

Is this merged to main branch already?

@pixxio
Copy link

pixxio commented Feb 29, 2016

Hey @DMcNamara ,

when will these changes be available in the master branch?
I am really looking forward to the new features.
I am not able to use the new awesome features until it is merged to master, am I?
A short answer would be highly appreciated!
Thanks

@allie-wake-up
Copy link
Contributor

You can install the plugin from @hanssens or @EddyVerbruggen repos to use the new GMImagePicker. I looked into pulling this in and unfortunately there are a few things I'd want to change to use it that I just don't have the time to do right now. I think we will pull this in eventually, but it could take quite a long time. There are a lot of great changes in it though.

@pixxio
Copy link

pixxio commented Mar 1, 2016

@TheKiteEatingTree
Thanks for your reply! I tried using @hanssens and @EddyVerbruggen repo but I really need the paths to the original files (I need the EXIF data). And as far as I have seen, his repo returns only the paths to the thumbnails, doesn't it?
There are many repos out there that provide great features but as far as I know yours is the only one that combines all features I need for a file picker.
I need these features:

  • multi selection
  • video selection
  • paths to original files

I would really really really appreciate if you could merge into master branch in the near future.

Regards

@vinlim
Copy link

vinlim commented Jul 17, 2016

+1

@Spriz
Copy link

Spriz commented Jan 18, 2017

Any news/plans/roadmap on this @DMcNamara / @sdushay ? :)

Vinoth18 pushed a commit to Vinoth18/cordova-imagePicker that referenced this pull request Dec 11, 2017
@hanssens hanssens closed this Sep 9, 2019
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.