Added callback function to be called on every single image load

[lmeurs]

Hi, I am new to Git, so please bare with me! :-)

I added a function called
imageLoaded( singleImageCallback [ , allImagesCallback ] )
The first callback argument gets executed everytime an image is loaded. The optional latter callback shows default imagesLoaded() behaviour.

I hope this addition is of any use!

Cheers, Laurens

[desandro]

@lmeurs - Welcome to the party! This is a nice idea, but I'm fairly sure that the single image callback is superfluous to using the progress method on the deferred object. @darsain can you confirm?

[lmeurs]

@desandro Thanks for the warm welcome! :-)

I agree with you, but strongly think a good image loader like this is a necessity to many people. My guess is that it is important to make this script easy to integrate in any website, a jQuery v1.7 dependency solely for dfd.progess() might be too high.

Also, the function called by dfd.progess() does not know which image was the trigger. An extra argument containing the trigger image or index number could be handy, but I suppose then dfd.progess() should be called from within the timeout as well.

The first callback argument of imageLoaded() is called on the single image HTML element and has $all_images, loaded_count, proper_count and broken_count as arguments. The second optional callback argument shows default imagesLoaded behavior, so is called on the initial jQuery collection.

And I hope I'm not being too bold here :-) , but I think it might be nice for the dfd.progess() and imageLoaded() callbacks to not receive array lengths, but jQuery collections, like all other callbacks do. The callback functions can get the collection's lengths themselves. I understand that a change like this this results in incompatibility when upgrading, but I just wanted to have it said!

[darsain]

Per image callback would be definitely too superfluous, as 99% of the people would never use it. Especially when there is .progress() method that does exactly that.

I'm looking at it like this: callback is for simple usage, while deferred is for advanced usage. So lets not go backwards, and bloat the plugin by porting deferred functionality into callbacks.

Also, introducing another .imageLoaded namespace is definitely not a good idea. We should stick to jQuery standards and keep one namespace per plugin. Especially regarding the feature that is already in plugin's deferred functionality. This would be just unnecessary bloating for the sake of tiny percentage of dinosaurs wanting to use this. jQuery 1.7 has been here for almost 4 months now. By the internet rules it is considered ancient :)

Instead, we should have a discussion on .progress() arguments, as I think you might be right that it doesn't provide enough.

In my first local version using deferred, I had .progress() receive the concerned image in a jQuery object as a function scope (this), and whole objects of proper/broken/loaded images as arguments, along with info whether the right now loaded image is broken or proper, ... basically EVERYTHING ... but it just seemed like too much and redundant. I couldn't think of a single usage case where I'd need that in there, and it wouldn't make more sense doing that in .done or .fail instead, so I decided to keep it simple, and let the progress be what it is supposed to be - a notification about loading progress.

I am for changing progress arguments, but only if there will be a good reason for it.

So the question is: What per image execution usage case can you imagine, that would need objects as arguments, and wouldn't make bigger sense doing in .done, .fail or .always?

For example: Replacing broken images with placeholders in .progress would be stupid, as whatever reason you had to call .imagesLoaded in the first place will be transferred into the placeholder, and you'd have to initiate .imagesLoaded on every single replacement, which would be simply too messy, and bad practice. This makes a lot bigger sense doing with .fail method on all broken images at once.

As far as I could think of, all usage cases made bigger sense with .done, .fail or .always. Well, except one - marking images in plugin demonstration with proper/broken class while they are being loaded :) But that is not worth un-simplifying the progress method, and seems more like a feature for the sake of feature, which I hate in all plugins developed this way.

[darsain]

And, if someone really needs per image loaded execution, they can always do:

$("img").each(function(i,e){
  $(e).imagesLoaded(function(){
    // code
  });
});

Which is even less code than code added in your pull request to introduce this :)

[lmeurs]

I understand where you are coming from, thank you for the quick replies.

The jQuery v1.7 requirement is a bit high for some users using other scripts that depend on another version of jQuery. Drupal for instance is still sticking at v1.5.2. As I said, in my opinion a good image loader should reach a broad(er) audience.

I stumbled upon this script when looking for an updated version of https://github.com/peol/jquery.imgloaded. imagesLoaded() apparently serves another purpose, but could easily be doing the same as imgloaded(). But better. And more.

Most of the times we don't care when an image is done loading, but sometimes we do. In case of a (pre)loader or when you want to take particular actions as soon as one or all images are loaded. Images could be hidden at first (slideshows, animations, etc.) and shown or re-positioned when they get loaded, instead of having to wait for the whole bunch to be done loading.

I started out with something like your last example, but to me it seemed more versatile if we also knew the number of loaded images and perhaps could easily add a function when all images are loaded. So I came up with a similar external version:

$.fn.imageLoaded = function( single_callback , total_callback ) {
  var $this = this,
    $_images = $this.find('img').add( $this.filter('img') ),
    counter = 0;

  return $this.each(function( i , image ) {
    var $image = $(image);
    $image.imagesLoaded(function($images, $proper, $broken) {// $images only contains this single image
      // execute single_callback on every single image load
      single_callback.call( $image, $_images, ++counter );

      // execute optional total_callback when all images are loaded
      if ($_images.length===counter && $.isFunction(total_callback)) total_callback.call( $this , $_images, $proper, $broken);
    });
  });
};

Not the way to go, I reckon! Yes, I am aware of my 'ability' to overthink too many scenarios and sometimes end up with bloatious code. ;-)

I did not know of namespace polution, but understand what you mean. In that case I propose to rename the project to imageLoaded() with two callbacks, all in favor? :-)

[darsain]

The primary plugin role is expressed properly with its current name. I don't see the reason for renaming it.

However, I've reconsidered progress arguments, and I think it might be beneficial to provide full jQuery objects with image elements. My proposition is to trigger progress with:

deferred.notifyWith( $loaded_image, [
    isBroken,        // boolean status of $loaded_image
    $all_images,     // all images in stack
    $proper_images,  // thus far properly loaded images
    $broken_images   // thus far broken images
] );

With that in mind, the plugin requirements are almost nonexistent. .imagesLoaded works in almost all jQuery versions there are. You are talking only about .progress method, and I don't want to bloat the plugin with support for old jQuery versions regarding supplementary functionality, i.e. functionality not at all crucial to the plugin primary role, which is to trigger a callback when all images has finished with loading. I already think that this plugin is too big. At least from where it was before I touched it :)

And don't get me wrong, after looking at it more thoroughly I see the benefit of per image callbacks and I'm OK with introducing the necessary arguments into .progress, but I don't want to add more .imagesLoaded() argments and do things in code twice, just to support supplementary features in old versions. jQuery is not IE :)

So that is my opinion: Yes for improving progress arguments; No for extending plugin with more arguments and callbacks for old jQuery support regarding a feature that will be mostly unused.

It's not like that feature is not there, you just need at least 4 months old jQuery to use it :)

And I've never worked with Drupal, but if you can't upgrade jQuery for non-admin areas, than it's really a shame.

[lmeurs]

I hope you understood that I was far from serious about renaming your project...

Calling deferred.notifyWith() would greatly add to the functionality of the script!

And as a 'final thought', please allow to play the devil's advocate (after this I will rest my case, promise :-): if the per image callback is superfluous, isn't the initial callback a bit redundant as well?

Thanks for your time, I learned a lot!

[darsain]

The initial callback is the point of the plugin, while deferred is the bonus for advanced users wanting more power. And .progress is a bonus of deferred functionality. So what you want is to bloat the plugin with support for old jQuery versions because the bonus of a bonus doesn't work in them :)

Not to mention that it would create this kinda calling:

$("img").imagesLoaded( null, function(){} );

Which is ugly, and it makes more sense doing like:

$("img").imagesLoaded({ allImages: function(){}, singleImage: function(){} });

But than, 99% of the people not using single callback would be inconvenienced with passing callback in options object.

Right now it is nice and dandy, with all functionality included. The only problem is the .progress method and old jQuery versions, which is just not worth the issues mentioned above. And I don't believe that updating jQuery can be mission impossible. Especially when jQuery is rarely backwards incompatible.

I have no problem with extending the plugin with new and useful feature. But extending it with bonus functionality that is already included, just so the little percentage of people wanting to use it & running on an old jQuery version wouldn't have to update ... it just doesn't seem right :)

[desandro]

@lmeurs Thank you for the request and for the discussion. As @darsain has explained, I don't think this is a good fit for the project. However, you have some good ideas and identified a feature gap.

I've opened #22 to discuss targeting certain images.