Loop: images land on the last field only

Question

I have a little problem with a loop. I am building a little tool, where a user must upload 12 images. The images are cropped in rectangles and placed on buttons. I am almost ready, but somehow the loop doesn't work well. All images land on the last button. Maybe something wrong in the loop here?

JS/JQuery:

for (var i = 0; i < 12; i++) {
    var j=i+1;
    var reader = new FileReader();
    reader.onload = function (e) {
        var img = new Image();
        img.src = e.target.result;

        img.onload = function () {

            var getimage= '#getimage'+j;

            // CREATE A CANVAS ELEMENT AND ASSIGN THE IMAGES TO IT.
            var canvas = document.createElement("canvas");

            var ctx = canvas.getContext("2d");
            ctx.clearRect(0, 0, canvas.width, canvas.height)
            var posh, posw;
            var factheight=img.height;
            var factwidth=img.width;
            if(factwidth<factheight){
                canvas.width = img.width;
                canvas.height= img.width;
                posh=(img.height-img.width)/2;
                posw=0;
            }
            else if(factheight<factwidth){

                canvas.height = img.height;
                canvas.width = img.height;
                posh=0;
                posw=(img.width-img.height)/2;
            }
            else{
                canvas.width = img.width;
                canvas.height= img.height;
                posh=0;
                posw=0;
            }
            ctx.drawImage(img, posw, posh, canvas.width, canvas.height, 0, 0, canvas.width, canvas.height);

            var cropped=canvas.toDataURL("image/png");

            $(getimage).attr("src",cropped);     // SHOW THE IMAGES OF THE BROWSER.
        }
    }

    reader.readAsDataURL($('.multiupload')[0].files[i]);

 }

Here is also a link to the JSFiddle. Appreciate your help, since I don't know exactly how reader.readAsDataURL($('.multiupload')[0].files[i]); and target.result works

Possible duplicate of [JavaScript closure inside loops – simple practical example](http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — Teemu, Jan 16 '17 at 16:49
You can't use j inside onload. Because onload will be called after loop is ended (as it is callback - async function). Every onload function call will have j=12. onload = (function(j){ return function() { ...your code } })(j) may help you. Maybe someone will finish my thoughts.h — Shulyk Volodymyr, Jan 16 '17 at 17:08
In the first line of the onload function add `var img = this;` will get the correct image. But the last line of the onload function will not work as mentioned above j will have the wrong value. — Blindman67, Jan 17 '17 at 03:00

score 1 · Answer 1 · answered Jan 16 '17 at 17:10

1

I'm guessing that your loop has finished before any of the images are fully loaded so j will be 11 before its used to find the relevant button. Try changing

img.onload = function () { .... }

to

img.onload = myFunction(id)

Then move everything out of the inline function into its own function with an input parameter. Then pass j as the id param.

answered Jan 16 '17 at 17:10

cspete

170
8

Thanks for your answer. Yes, the images are loading very slowly, but since I am using the cropper in the background it won't be fast. Do you know how I could apply a loading image until the images are fully loaded? – PLAYCUBE Jan 16 '17 at 17:31
1

Here you go. https://jsfiddle.net/53aesgep/2/ Just call hideLoader when `j` is 11 (or you max number of images) – cspete Jan 18 '17 at 11:08

score 1 · Answer 2 · answered Jan 16 '17 at 17:18

1

I've done an example for you. As I answered in comments

var reader = new FileReader();
reader.onload = (function(j){return function (e) {
var img = new Image();
...

https://jsfiddle.net/ykze3f9r/

answered Jan 16 '17 at 17:18

Shulyk Volodymyr

767
6
11

Thank you! I just managed to do this like this `var funcs=[]; function multicrop(i){ ... }` and `for (var i = 0; i < 6; i++) { funcs[i]=multicrop(i); }` Is either way good? – PLAYCUBE Jan 16 '17 at 17:33
It works, i agree. But looks not very elegant. That's your implementation - so it's the best) – Shulyk Volodymyr Jan 17 '17 at 08:12

score 1 · Answer 3 · answered Jan 16 '17 at 17:55

The main issue with the code was the j variable. It was always set to the last number because of the way for loops work. You have to instead bind that number. I broke up into separate functions to make it easier to read. Here's the working JSFiddler: https://jsfiddle.net/eh6pr7ee/2/

Processes the image...

var processImg = function( img, imgNum ) {
  var getimage= '#getimage' + imgNum;
  // CREATE A CANVAS ELEMENT AND ASSIGN THE IMAGES TO IT.
  var canvas = document.createElement("canvas");

  var ctx = canvas.getContext("2d");
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  var posh, posw;
  var factheight = img.height;
  var factwidth = img.width;
  if (factwidth < factheight) {
    canvas.width = img.width;
    canvas.height = img.width;
    posh = (img.height-img.width)/2;
    posw = 0;
  }
  else if (factheight < factwidth) {
    canvas.height = img.height;
    canvas.width = img.height;
    posh = 0;
    posw = (img.width-img.height)/2;
  }
  else {
    canvas.width = img.width;
    canvas.height= img.height;
    posh = 0;
    posw = 0;
  }
  ctx.drawImage(img, posw, posh, canvas.width, canvas.height, 0, 0, canvas.width, canvas.height);
  var cropped = canvas.toDataURL("image/png");
  $(getimage).attr("src",cropped); // SHOW THE IMAGES OF THE BROWSER.
};

Creates image and sets source...

var setImage = function( imgNum, e ) {
  var img = new Image();
  img.src = e.target.result;
  img.onload = processImg.bind( this, img, imgNum );
};

Create a handler function for image uploads...

var handleImageUploads = function() {
  if (parseInt($(this).get(0).files.length) > 12 || parseInt($(this).get(0).files.length) < 12) {
    alert("Please upload 12 photos");
  }
  else {
    //loop for each file selected for uploaded.
    for (var i = 0; i < 12; i++) {
      var reader = new FileReader();
      reader.onload = setImage.bind( this, i+1 );
      reader.readAsDataURL($('.multiupload')[0].files[i]);
    } // for
    console.log("done");
    $('body').removeClass("loading");
  }; // else
}

Binds the handler function.

$('.multiupload').on("change", handleImageUploads);

Loop: images land on the last field only

3 Answers3