desandro / imagesloaded

:camera: JavaScript is all like "You images done yet or what?"
https://imagesloaded.desandro.com
MIT License
8.88k stars 1.15k forks source link

Reload of broken images doesn't seem to be supported #43

Closed dotnetCarpenter closed 11 years ago

dotnetCarpenter commented 11 years ago

I'm making a photo collage and using an unreliable service while developing so I often have broken images. I try to remedy that by reloading the images with (using jQuery)

$('#content-scene img').imagesLoaded(function($images, $proper, $broken) {
    // reload broken images
    $broken.each(function(i, img) {
       img.src += '?reload=' + new Date().getTime();
    });
   ...

I then do some logic with the $proper images. My expectation is that the function would be called again when the $broken images are loaded but it never happens. Is this a flaw in my expectation or a bug in imagesloaded?

I don't have time to dig into this right now but will update this with a test case as soon as possible. Probably later tonight.

Cheers, Jon.

PS. Link to dev page: https://c9.io/dotnetcarpenter/mireia/workspace/newsite/collections.html (not guarantied to be working in your browser ;-s )

darsain commented 11 years ago

Callback is a "Function that will be called when all images has finished with loading, regardless of their final state (proper/broken)."

That's it. When all images from $('#content-scene img') has finished with loading (doesn't matter if they are properly loaded, or browser gave up and marked them as broken), your callback will be fired. That's the whole meaning of this plugin. There is no event delegation, as it would make no sense in the context/purpose of the plugin.

So to do what you want, you should do something like:

function fixBroken(broken) {
    $(broken).on('error', function () {
        fixBroken(this);
    }).each(function(i, img) {
        // update src logic here
    });
}

$('#content-scene img').imagesLoaded(function($images, $proper, $broken) {
    fixBroken($broken);
});

It's a recursive solution that will keep trying to reload broken images. The fact that this might not be a best thing to do is another issue...

dotnetCarpenter commented 11 years ago

Thanks Darsain. I'm curious to why you think, it might not be a good idea? I can see how multiple error event listeners could be attached in your example and that reloads should be limited to e.g. 1-2 reloads but other than that, your example looks good to me. I've tried to fix the cons mentioned above by doing this: EDIT: solution is flawed - will only reload the images once

function fixBrokenImages($broken) {
    $broken.one('error', function () { // <- notice the "one" method
        if( $(this).data('r') < 3 ) { // only try twice
            fixBrokenImages(this);
        }
    }).each(function(i, img) {                
        img.src += '?reload=' + (($(img).data('r') || 0)+1);
    });
}
function positionImages($proper) {
    // find and set the coordinates of loaded images
    $proper.each(function(i, img){
        $(img).css({
            top: img.offsetTop,
            left:img.offsetLeft
        });
    });
    $proper.addClass('positioned');
}
$('#content-scene img').imagesLoaded(function($images, $proper, $broken) {
    // reload broken images
    fixBrokenImages($broken);
    // position images
    positionImages($images);
});

I've yet to see an image fail twice but I think my logic is correct. Does this address your mention concern?

darsain commented 11 years ago

My concern was regarding you using such an unreliable host, and instead of trying to solve it, you are "fixing" it by making tons of new http requests. That's just bad on top of bad.

dotnetCarpenter commented 11 years ago

Yes, your right. But for development it's ok. I'm doing this project as a favor to a friend, and in the end it will be her call, if she wants to buy a dedicated server to handle image resizing on the fly. IMHO will be a better solution. I could also cave in and implement it in PHP on her current server host but I rather not.

Anyhow, I need something that works now for development and at any rate this is an improvement to the script, even if it is symptom treatment.

Thank you so much for attending to my issue. It's rare that non bugs get any positive attention on github. Cheers

dotnetCarpenter commented 11 years ago

Wow, I just saw recurring reload failures for the first time and $broken.one('error' seems to never fire (in Firefox 17.0.1). Any thoughts on that one?

dotnetCarpenter commented 11 years ago

For the sake of history and perhaps helping someone else, here is what I ended up with using.

function fixBrokenImages($broken) {
    var re = /\?reload=(\d)$/
    $broken.one('error', function () {
        if( this.src.match(re)[1] < 3 ) {
            fixBrokenImages($(this));
        }
    }).each(function(i, img) {
        var tries = img.src.match(re);
        img.src = img.src.replace(re, '') + '?reload=' + (tries ? ++tries[1] : 1);
        console.log("tries: %s \nsrc: %s", (tries ? tries[1] : 1), img.src);
    });
}
$('#content-scene img').imagesLoaded(function($images, $proper, $broken) {
    // reload broken images
    fixBrokenImages($broken);
    // position images
    positionImages($images);
});

Note that jQuery.data() seems to be confused about what element you attach data to, if a) it's an image and b) you change the src.

designbyadrian commented 11 years ago

I'm having a similar issue where the browser (iPad) can take several minutes before reporting an image as broken, most likely because of conjested network. How can i force reject imageLoaded after a set timeout? I can't reject the promise object...

darsain commented 11 years ago

Adding a timeout is on a to-do list. Along with other stuff... It'll need API re-factoring, as we will have to report 3rd array with pending images in callbacks.

designbyadrian commented 11 years ago

Sounds good! I have a fairly OK solution for now. Quite tied to my app, so I can't share it, but it's based apon dotnetCarpenter's solution.