cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dieppe <...@git.apache.org>
Subject [GitHub] cordova-plugin-camera pull request: Major refactor
Date Fri, 05 Jun 2015 17:26:42 GMT
GitHub user dieppe opened a pull request:

    https://github.com/apache/cordova-plugin-camera/pull/101

    Major refactor

    Hi there,
    
    We needed access to the metadata which lead to asynchronous operations. We took the opportunity
to refactor most of the plugin logic. Hopefully it's more readable and actually fixes some
known bugs (e.g. we deal properly with NATIVE_URI wherever possible).
    
    This is a call for feedback and testing while I try to fix a known bug.
    
    The commit message is quite explicit, so here it is:
    
    ---
    The major thing added here is that metadata is correctly handled in most (all?) cases.
I also think some bugs have been fixed, but I did not check the bug list ;)
    
    As a foreword, I want to say that I am far from being an expert in either Objective-C
nor the iOS APIs (I only did some basic patches before). So please correct me if I say/did
anything wrong.
    
    # Global flow
    As a general design decision, I tried to break all the workflow in small readable methods
that either return something (I chose to use reference passing when returning more than one
result which I find clearer than returning an NSDictionary where the result is specified nowhere),
or use the convention completionBlock / resultBlock / failureBlock which it seems Apple APIs
use.
    
    There are three main paths from the image retrieval to the result.
    
    1. The user wants the original image from the PhotoLibrary or from the SavedPhotoAlbum
with a destination type NATIVE_URI.
    2. The user wants a modified version of the image, wherever it comes from.
    3. The user wants the geolocation.
    
    # Option incompatibilities
    There are also some combinations of options that are disallowed because they don’t make
any sense:
    
    1. if the user wants an image with a source type CAMERA, and a destination type NATIVE_URI,
he has to save it to the photo album. An image that is not there does not have a NATIVE_URI.
    2. if the user wants an image with a source type PHOTOLIBRARY or SAVEDPHOTOALBUM, needs
editing (resize, orientation correction), and a destination type NATIVE_URI, he has to save
it to the photo album. Otherwise, we would return the original image with no modification.
    
    As we can see, if we want a destination type of NATIVE_URI, there are only two possible
paths: either we want an unmodified version of a library image, or we need to save to the
photo album.
    
    # Metadata
    Regarding metadata, we do not need it in only one case, when the destination type is NATIVE_URI,
the source is the library, and no edits have to be done.
    
    Otherwise, we need to either fetch if from the UIImagePickerControllerMediaMetadata key
of the info dictionary provided by UIImagePickerController in case the source type is CAMERA,
or from the ALAsset corresponding to the library image (see https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIImagePickerControllerDelegate_Protocol/index.html#//apple_ref/doc/constant_group/Editing_Information_Keys).
 
    This actually introduces asynchronicity in the metadata retrieval, since we first need
to fetch the ALAsset using UIImagePickerControllerReferenceURL. *We cannot use UIImage as
it strips out most (all?) of the metadata*.
    
    Once we actually retrieved the metadata we need to modify it for every transformation
of the image (orientation, resize, gps).
    
    # Saving to the Photo Album
    In the previous code, the saving was done after computing the CDVResult. This is not the
case anymore as we need access to the saved ALAsset when we want to return a NATIVE_URI. The
saving is now done after all image editing has occurred.
    
    # Asynchronous processing
    Since the metadata retrieval and the album saving is done asynchronously, the process
pipeline itself is made asynchronous. We can use resultBlock and failureBlock to get the processed
image with its corresponding metadata or to get the image Asset NATIVE_URI.
    
    # FIXMEs
    The documentation lacks some clarity for two things (see FIXME 1 & 2 in code):
    
    -  Does the quality option apply to the library images as well. This would mean that we
cannot use NATIVE_URI with the quality set, except if we save to the photo album.
    - Does the orientation correction apply to the library images as well. Same thing. I actually
removed that options at first, but this would mean that we need to read the EXIF data in the
js code and orient the image appropriately (except for NATIVE_URIs since the web view does
that automagically, but orientation correction does not make sense here). 
    
    # TODOs
    - usesGeolocation does not make sense for an image with a source type other that CAMERA
(maybe on some fringe cases?)
    - fix a bug where allowsEdit + saveToPhotoAlbum + NATIVE_URI make the image save in the
wrong orientation (despite the metadata having the correct orientation afaict)
    
    WARNING:
    I did not test the usesGeolocation option due to a lack of time (and, frankly, a lack
of interest;).
    
    ACKNOWLEDGMENT:
    Shoutout to @athibaud who also took part in the refactoring effort. Any bug’s on him!
;)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jnuine/cordova-plugin-camera master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cordova-plugin-camera/pull/101.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #101
    
----
commit 8277888b1884c7881b543df8143828e17907e316
Author: Clément Vollet <clement@jnuine.com>
Date:   2015-06-05T16:46:49Z

    Major refactor
    
    The major thing added here is that metadata is correctly handled in
    most (all?) cases. I also think some bugs have been fixed, but I did
    not check the bug list ;)
    
    As a foreword, I want to say that I am far from being an expert in
    either Objective-C nor the iOS APIs (I only did some basic patches
    before). So please correct me if I say/did anything wrong.
    
    #Global flow
    As a general design decision, I tried to break all the workflow in
    small readable methods that either return something (I chose to use
    reference passing when returning more than one result which I find
    clearer than returning an NSDictionary where the result is specified
    nowhere), or use the convention completionBlock / resultBlock /
    failureBlock which it seems Apple APIs use.
    
    There are three main paths from the image retrieval to the result.
    
    1. The user wants the original image from the PhotoLibrary or from the
    SavedPhotoAlbum with a destination type NATIVE_URI.
    2. The user wants a modified version of the image, wherever it comes
    from.
    3. The user wants the geolocation.
    
    #Option incompatibilities
    There are also some combinations of options that are disallowed because
    they don’t make any sense:
    1. if the user wants an image with a source type CAMERA, and a
    destination type NATIVE_URI, he has to save it to the photo album. An
    image that is not there does not have a NATIVE_URI.
    2. if the user wants an image with a source type PHOTOLIBRARY or
    SAVEDPHOTOALBUM, needs editing (resize, orientation correction), and a
    destination type NATIVE_URI, he has to save it to the photo album.
    Otherwise, we would return the original image with no modification.
    
    As we can see, if we want a destination type of NATIVE_URI, there are
    only two possible paths: either we want an unmodified version of a
    library image, or we need to save to the photo album.
    
    #Metadata
    Regarding metadata, we do not need it in only one case, when the
    destination type is NATIVE_URI, the source is the library, and no edits
    have to be done.
    
    Otherwise, we need to either fetch if from the
    UIImagePickerControllerMediaMetadata key of the info dictionary
    provided by UIImagePickerController in case the source type is CAMERA,
    or from the ALAsset corresponding to the library image (see
    https://developer.apple.com/library/ios/documentation/UIKit/Reference/UI
    ImagePickerControllerDelegate_Protocol/index.html#//apple_ref/doc/consta
    nt_group/Editing_Information_Keys).
    This actually introduces asynchronicity in the metadata retrieval,
    since we first need to fetch the ALAsset using
    UIImagePickerControllerReferenceURL. *We cannot use UIImage as it
    strips out most (all?) of the metadata*.
    
    Once we actually retrieved the metadata we need to modify it for every
    transformation of the image (orientation, resize, gps).
    
    #Saving to the Photo Album
    In the previous code, the saving was done after computing the
    CDVResult. This is not the case anymore as we need access to the saved
    ALAsset when we want to return a NATIVE_URI. The saving is now done
    after all image editing has occurred.
    
    #Asynchronous processing
    Since the metadata retrieval and the album saving is done
    asynchronously, the process pipeline itself is made asynchronous. We
    can use resultBlock and failureBlock to get the processed image with
    its corresponding metadata or to get the image Asset NATIVE_URI.
    
    #CropToSize
    CropToSize is not a valid option (it is always set to NO anyway) so it
    has been removed.
    
    #FIXMEs
    The documentation lacks some clarity for two things (see FIXME #1 & #2):
    -  Does the quality option apply to the library images as well. This
    would mean that we cannot use NATIVE_URI with the quality set, except
    if we save to the photo album.
    - Does the orientation correction apply to the library images as well.
    Same thing. I actually removed that options at first, but this would
    mean that we need to read the EXIF data in the js code and orient the
    image appropriately (except for NATIVE_URIs since the web view does
    that automagically, but orientation correction does not make sense
    here).
    
    #TODOs
    - usesGeolocation does not make sense for an image with a source type
    other that CAMERA (maybe on some fringe cases?)
    - fix a bug where allowsEdit + saveToPhotoAlbum + NATIVE_URI make the
    image save in the wrong orientation (despite the metadata having the
    correct orientation afaict)
    
    WARNING:
    I did not test the usesGeolocation option due to a lack of time (and,
    frankly, a lack of interest;).
    
    ACKNOWLEDGMENT:
    Shoutout to @athibaud who also took part in the refactoring effort. Any
    bug’s on him! ;)

commit 7e30f5f9131add3469da397aeefbdbfd1e95e571
Author: Clément Vollet <clement@jnuine.com>
Date:   2015-06-05T16:47:45Z

    Fix resize test
    
    Target size needs to be different for portrait and landscape (or the
    XCTAssert is wrong).

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


Mime
View raw message