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

[Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component #1318

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@ptmt
Copy link
Contributor

ptmt commented May 17, 2015

This PR adds 4 native events to NetworkImage.

demo

Using these events I could wrap Image component into something like:

class NetworkImage extends React.Component {
  getInitialState() {
    return {
      downloading: false,
      progress: 0
    }
  }

  render() {
    var loader = this.state.downloading ?
      <View style={this.props.loaderStyles}>
        <ActivityIndicatorIOS animating={true} size={'large'} />
        <Text style={{color: '#bbb'}}>{this.state.progress}%</Text>
      </View>
      :
      null;

    return <Image source={this.props.source}
      onLoadStart={() => this.setState({downloading: true}) }
      onLoaded={() => this.setState({downloading: false}) }
      onLoadProgress={(e)=> this.setState({progress: Math.round(100 * e.nativeEvent.written / e.nativeEvent.total)});
      onLoadError={(e)=> {
        alert('the image cannot be downloaded because: ', JSON.stringify(e));
        this.setState({downloading: false});
      }}>
      {loader}
    </Image>
  }
}

Useful on slow connections and server errors.

There are dozen lines of Objective C, which I don't have experience with. There are neither specific tests nor documentation yet. And I do realize that you're already working right now on better <Image/> (pipeline, new asset management, etc.). So this is basically a proof concept of events for images, and if this idea is not completely wrong I could improve it or help somehow.


@interface RCTDownloadTaskWrapper : NSObject <NSURLSessionDownloadDelegate>

@property(nonatomic,assign) id<NSURLSessionDownloadDelegate> delegate;

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Nit: One space after @​property.

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Delegates should have "weak" retain semantics.



// completionBlock will contain the block you're going to call when the request is completed
@property (copy, nonatomic) RCTDataCompletionBlock completionBlock; //void (^completionBlock)(RCTDataCompletionBlock *);

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Remove comment. The typedef is enough.

@property (copy, nonatomic) RCTDataCompletionBlock completionBlock; //void (^completionBlock)(RCTDataCompletionBlock *);

// progressBlock will contain the block to trigger a downloadProgress event
@property (copy, nonatomic) RCTDataProgressBlock progressBlock;//void (^progressBlock)(RCTDataProgressBlock);

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Same.



// Create download task
- (NSURLSessionDownloadTask *) downloadData:(NSURL *) url

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

I would recommend creating an -init method that takes in an NSURLSession, rather than creating a new one every time.

This comment has been minimized.

@ptmt

ptmt May 19, 2015

Author Contributor

I don't understand how to pass the right delegate property for shared session in that case. I mean this line NSURLSession *session = [NSURLSession sessionWithConfiguration:conf delegate:self delegateQueue:nil];

This comment has been minimized.

@a2

a2 May 19, 2015

Contributor

Ah, I see. Perhaps passing in the session configuration and delegate queue?

- (instanceType)initWithSessionConfiguration:(NSURLSessionConfiguration *)configuration delegateQueue:(NSOperationQueue *)delegateQueue;

This comment has been minimized.

@ptmt

ptmt May 19, 2015

Author Contributor

Still can't figure out. Need digging into documentation and the examples.

- (void) URLSession:(NSURLSession *)session downloadTask:(NSURLSessionDownloadTask *)downloadTask didFinishDownloadingToURL:(NSURL *)location
{
NSData *data = [NSData dataWithContentsOfURL:location];
NSLog(@"Downloaded: %lu", (unsigned long)data.length);

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Remove log

NSString *cacheKey = RCTCacheKeyForURL(url);

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Remove trailing whitespace

@@ -27,6 +31,12 @@
*/
@property (nonatomic, strong) NSURL *imageURL;

///**

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Remove?

@@ -22,6 +25,8 @@ @implementation RCTNetworkImageView
NSUInteger _deferSentinel;
RCTImageDownloader *_imageDownloader;
id _downloadToken;
/* Required to publish events */

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

If required, put as trailing comment after the ivar declaration.

@@ -58,8 +74,35 @@ - (void)setImageURL:(NSURL *)imageURL resetToDefaultImageWhileLoading:(BOOL)rese
self.layer.minificationFilter = kCAFilterTrilinear;
self.layer.magnificationFilter = kCAFilterTrilinear;
}
NSDictionary *event = @{

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Please reformat these dictionary literals

This comment has been minimized.

@ptmt

ptmt May 19, 2015

Author Contributor

Sorry, XCode makes me crazy about formatting thinks like this. Used this https://github.com/github/objective-c-style-guide#literals

This comment has been minimized.

@a2

a2 May 19, 2015

Contributor

It's totally fine. Yeah, Xcode is crazy when formatting literals like these.

@@ -31,4 +36,22 @@ - (UIView *)view
RCT_REMAP_VIEW_PROPERTY(src, imageURL, NSURL)
RCT_EXPORT_VIEW_PROPERTY(contentMode, UIViewContentMode)

- (NSDictionary *)customDirectEventTypes
{
return @{

This comment has been minimized.

@a2

a2 May 18, 2015

Contributor

Here as well

@a2

This comment has been minimized.

Copy link
Contributor

a2 commented May 18, 2015

Sorry for all the nitpicks.

@a2 a2 self-assigned this May 18, 2015

@ptmt

This comment has been minimized.

Copy link
Contributor Author

ptmt commented May 19, 2015

Thank you so much! Could you help me about tests and other possible performances issues? As you mentioned before NSURLSession which creating each time. For now I use it in the app with a lot of images loading - everything is fine, but I understand these changes could affect somebody. Or should I move it in external library?

{
NSString *cacheKey = RCTCacheKeyForURL(url);
__weak RCTImageDownloader *weakSelf = self;
return [self _downloadDataForURL:url block:^(BOOL cached, NSData *data, NSError *error) {
return [self _downloadDataForURL:url progressBlock:(RCTDataProgressBlock)progressBlock block:^(BOOL cached, NSData *data, NSError *error) {

This comment has been minimized.

@a2

a2 May 19, 2015

Contributor

You don't need to cast the blocks.

This comment has been minimized.

@ptmt

ptmt May 19, 2015

Author Contributor

ok

@property (copy, nonatomic) RCTDataCompletionBlock completionBlock;

@property (copy, nonatomic) RCTDataProgressBlock progressBlock;

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor
- (instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration *)configuration delegateQueue:(NSOperationQueue *)delegateQueue;

#import "RCTDownloadTaskWrapper.h"

@implementation RCTDownloadTaskWrapper

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor
{
  NSURLSession *_URLSession;
}

@implementation RCTDownloadTaskWrapper

- (NSURLSessionDownloadTask *)downloadData:(NSURL *)url

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor
- (instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration *)configuration delegateQueue:(NSOperationQueue *)delegateQueue
{
  if ((self = [super init])) {
    _URLSession = [NSURLSession sessionWithConfiguration:configuration delegate:self delegateQueue:nil];
  }

  return self;
}
// setup the session (TODO: reuse sharedSession)
NSURLSession *session = [NSURLSession sessionWithConfiguration:conf delegate:self delegateQueue:nil];

NSURLSessionDownloadTask *task = [session downloadTaskWithURL:url completionHandler:nil];

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor

[_URLSession downloadTask...

@interface RCTDownloadTaskWrapper : NSObject <NSURLSessionDownloadDelegate>

@property (copy, nonatomic) RCTDataCompletionBlock completionBlock;

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor

Nit: Remove newline

@property (copy, nonatomic) RCTDataProgressBlock progressBlock;

// Create download task
- (NSURLSessionDownloadTask *) downloadData:(NSURL *) url

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor

Remove space after *) before downloadData. This method signature could be one line, actually.

{

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor

Remove these newlines

@"loadStart": @{ @"registrationName": @"onLoadStart" },
@"loadEnd": @{ @"registrationName": @"onLoadEnd" },
@"error": @{ @"registrationName": @"onError" },
@"downloadProgress": @{ @"registrationName": @"onDownloadProgress" }

This comment has been minimized.

@a2

a2 May 20, 2015

Contributor

Add trailing comma

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented May 27, 2015

Thanks for all of the work here people, looking forward to seeing this in 😄

@ptmt

This comment has been minimized.

Copy link
Contributor Author

ptmt commented May 29, 2015

Merged from upstream (new lines about resizing etc), I've got unnecessary commits, it looks unclean now. I use this branch in production, though, don't know what to do next exactly. Tests? Rebase?

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented May 29, 2015

@a2 - any chance you have a bit of time to ❤️ this PR?

@ide

This comment has been minimized.

Copy link
Collaborator

ide commented May 29, 2015

As for the API you may want to study https://html.spec.whatwg.org/multipage/embedded-content.html#mediaevents to see if any of the concepts are worth sharing (cc @vjeux)

@ptmt

This comment has been minimized.

Copy link
Contributor Author

ptmt commented May 30, 2015

@ide thanks, good point. I consider to adding the abort event. And about events' naming conventions, not sure how to do it right. I've used https://facebook.github.io/react-native/docs/textinput.html#onchange pattern on{eventName}. It's also similar to the current state of https://github.com/brentvatne/react-native-video, but it makes sense to rename methods. So we will have:
loadstart, progress, abort, error, loadeddata.

@brentvatne brentvatne changed the title Add onDownloadProgress, onError events to Image component [Image] Add onDownloadProgress, onError events to Image component May 30, 2015

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented May 31, 2015

@vjeux - can you shed any light on how you'd like to do naming for these types of events, as per @UnknownException comment above?

@browniefed

This comment has been minimized.

Copy link
Contributor

browniefed commented Jun 1, 2015

This would be super useful, especially if the loaded data got passed back on success in base64 encoded string or some workable format.

@a2

This comment has been minimized.

Copy link
Contributor

a2 commented Jun 1, 2015

Waiting on event naming convention comments.

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jun 1, 2015

I like the naming from the spec:

  • onLoadStart
  • onLoadProgress
  • onLoadAbort
  • onLoadError
  • onLoaded

This would be super useful, especially if the loaded data got passed back on success in base64 encoded string or some workable format.

No. We never want to read image data in js. This means sending a huge chunk of memory (several megabytes) to js. There are some places where js is not a good language and doing image processing is one of them. You should be manipulating the image using C or the GPU. Not js with a base64 encoded file.

_downloadToken = [_imageDownloader downloadImageForURL:imageURL size:self.bounds.size scale:RCTScreenScale() resizeMode:self.contentMode backgroundColor:self.backgroundColor block:^(UIImage *image, NSError *error) {
_downloadToken = [_imageDownloader downloadImageForURL:imageURL size:self.bounds.size scale:RCTScreenScale()
resizeMode:self.contentMode backgroundColor:self.backgroundColor
progressBlock:progressHandler block:^(UIImage *image, NSError *error) {

This comment has been minimized.

@vjeux

vjeux Jun 1, 2015

Contributor

Can you not update progress if there is no onProgress callback setup in js? This is wasteful to send events that are not going to be handled. (I'm not familiar with iOS so i'm not sure if you already do that, sorry if it's the case)

This comment has been minimized.

@ide

ide Jun 1, 2015

Collaborator

Agreed. There is a slightly tricky edge case to handle: on the Image's first render pass we don't provide an onProgress handler, and then provide one on the second pass. So the view/viewmanager needs to enable/disable the progress events when the onProgress handler prop is set/removed.

This comment has been minimized.

@ptmt

ptmt Jun 2, 2015

Author Contributor

Thanks for comments, I renamed events and tried to not fire the progress event if it's not defined.

Tested this behavior with something like this:

class TestNetworkImage extends Component {
  componentDidMount() {
      setTimeout(() => {
        this.setState({
          onProgress: this.onProgress.bind(this)
        })
      }, 1000);
    }

  render() {
    return <Image ref="image"
      source={{uri: this.props.source}}
      onLoadProgress={this.state.onProgress}>
      {this.props.loader}
      {this.props.children}
    </Image>
  }
}

NSLog shows that at first second ImageView doesn't send an event to js in that case, then it does.

@ptmt

This comment has been minimized.

Copy link
Contributor Author

ptmt commented Jul 7, 2015

The app which uses these changes has passed Apple review and available on AppStore. It's not a good thing to maintain the fork, so I need to figure out how to split it into the stand-alone plugin.

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented Jul 7, 2015

@UnknownException - I would love to see this merged upstream, @a2 - do you have any time to review again?

Also, @UnknownException - can you please rebase?

@a2

This comment has been minimized.

Copy link
Contributor

a2 commented Jul 7, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jul 7, 2015

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/470962509718878/int_phab to review.

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented Jul 8, 2015

❤️ @a2

@ptmt ptmt changed the title [Image] Add onDownloadProgress, onError events to Image component [Image] Add onLoadStart, onLoadProgress, onLoadError events to Image component Jul 8, 2015

@ptmt ptmt force-pushed the ptmt:feature/image_events branch from 1ea70c1 to 3aed7c2 Jul 8, 2015

@ptmt ptmt force-pushed the ptmt:feature/image_events branch from 3aed7c2 to 7c856f5 Jul 8, 2015

@ptmt

This comment has been minimized.

Copy link
Contributor Author

ptmt commented Jul 8, 2015

Synced with the master and rebased. I had to adopt the new caching mechanism (NSURLCache) to make this whole thing work with NSURLSessionDownloadTask instead of NSURLSessionDataTask. Please, review, if you can.
Should I add examples to UIExplore?

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented Jul 10, 2015

@UnknownException - if you can add an example to UIExplorer and submit in a separate PR that would be lovely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.