Copy paste bug : add unit tests for FrameUtils with null value

src/js/app.js

@@ -192,9 +192,7 @@
 
     getFirstFrameAsPng : function () {
       var firstFrame = this.piskelController.getFrameAt(0);
-      var canvasRenderer = new pskl.rendering.CanvasRenderer(firstFrame, 1);
+      var firstFrameCanvas = pskl.utils.FrameUtils.toImage(firstFrame);
-      canvasRenderer.drawTransparentAs('rgba(0,0,0,0)');
-      var firstFrameCanvas = canvasRenderer.render();
       return firstFrameCanvas.toDataURL('image/png');
     },
 

src/js/controller/preview/PreviewController.js

@@ -25,9 +25,6 @@
   };
 
   ns.PreviewController.prototype.init = function () {
-    // the oninput event won't work on IE10 unfortunately, but at least will provide a
-    // consistent behavior across all other browsers that support the input type range
-    // see https://bugzilla.mozilla.org/show_bug.cgi?id=853670
     this.fpsRangeInput.on('input change', this.onFPSSliderChange.bind(this));
     document.querySelector('.right-column').style.width = Constants.ANIMATED_PREVIEW_WIDTH + 'px';
 

src/js/rendering/CanvasRenderer.js

@@ -31,14 +31,6 @@
     return scaledCanvas;
   };
 
-  ns.CanvasRenderer.prototype.renderPixel_ = function (color, x, y, context) {
-    if (color == Constants.TRANSPARENT_COLOR) {
-      color = this.transparentColor_;
-    }
-    context.fillStyle = color;
-    context.fillRect(x, y, 1, 1);
-  };
-
   ns.CanvasRenderer.prototype.createCanvas_ = function (zoom) {
     zoom = zoom || 1;
     var width = this.frame.getWidth() * zoom;

src/js/selection/BaseSelection.js

@@ -24,13 +24,9 @@
   };
 
   ns.BaseSelection.prototype.fillSelectionFromFrame = function (targetFrame) {
-    // on copy trim the selection if out of bounds
-    // this.pixels = this.pixels.filter(function (pixel) {
-    //   return targetFrame.containsPixel(pixel.col, pixel.row);
-    // });
-
     this.pixels.forEach(function (pixel) {
-      pixel.color = targetFrame.getPixel(pixel.col, pixel.row);
+      var color = targetFrame.getPixel(pixel.col, pixel.row);
+      pixel.color  = color || Constants.TRANSPARENT_COLOR;
     });
 
     this.hasPastedContent = true;

src/js/selection/SelectionManager.js

@@ -88,20 +88,18 @@
   };
 
   ns.SelectionManager.prototype.paste = function() {
-    if (this.currentSelection && this.currentSelection.hasPastedContent) {
+    if (!this.currentSelection || !this.currentSelection.hasPastedContent) {
-      var pixels = this.currentSelection.pixels;
+      return;
-      var opaquePixels = pixels.filter(function (p) {
-        return p.color !== Constants.TRANSPARENT_COLOR;
-      });
-      this.pastePixels(opaquePixels);
     }
-  };
 
-  ns.SelectionManager.prototype.pastePixels = function(pixels) {
+    var pixels = this.currentSelection.pixels;
-    var currentFrame = this.piskelController.getCurrentFrame();
+    var frame = this.piskelController.getCurrentFrame();
 
     pixels.forEach(function (pixel) {
-      currentFrame.setPixel(pixel.col, pixel.row, pixel.color);
+      if (pixel.color === Constants.TRANSPARENT_COLOR || pixel.color === null) {
+        return;
+      }
+      frame.setPixel(pixel.col, pixel.row, pixel.color);
     });
 
     $.publish(Events.PISKEL_SAVE_STATE, {
@@ -115,8 +113,7 @@
   };
 
   ns.SelectionManager.prototype.replay = function (frame, replayData) {
-    var pixels = replayData.pixels;
+    replayData.pixels.forEach(function (pixel) {
-    pixels.forEach(function (pixel) {
       var color = replayData.type === SELECTION_REPLAY.PASTE ? pixel.color : Constants.TRANSPARENT_COLOR;
       frame.setPixel(pixel.col, pixel.row, color);
     });

src/js/utils/FrameUtils.js

@@ -5,8 +5,8 @@
     /**
      * Render a Frame object as an image.
      * Can optionally scale it (zoom)
-     * @param  {Frame} frame
+     * @param frame {Frame} frame
-     * @param  {Number} zoom
+     * @param zoom {Number} zoom
      * @return {Image}
      */
     toImage : function (frame, zoom) {
@@ -19,9 +19,9 @@
     /**
      * Draw the provided frame in a 2d canvas
      *
-     * @param  {pskl.model.Frame} frame the frame to draw
+     * @param frame {pskl.model.Frame} frame the frame to draw
-     * @param  {Canvas} canvas the canvas target
+     * @param canvas {Canvas} canvas the canvas target
-     * @param  {String} transparentColor (optional) color to use to represent transparent pixels.
+     * @param transparentColor {String} transparentColor (optional) color to use to represent transparent pixels.
      */
     drawToCanvas : function (frame, canvas, transparentColor) {
       var context = canvas.getContext('2d');
@@ -30,6 +30,9 @@
       for (var x = 0, width = frame.getWidth() ; x < width ; x++) {
         for (var y = 0, height = frame.getHeight() ; y < height ; y++) {
           var color = frame.getPixel(x, y);
+
+          // accumulate all the pixels of the same color to speed up rendering
+          // by reducting fillRect calls
           var w = 1;
           while (color === frame.getPixel(x, y + w) && (y + w) < height) {
             w++;
@@ -43,14 +46,23 @@
           y = y + w - 1;
         }
       }
-
     },
 
+    /**
+     * Render a line of a single color in a given canvas 2D context.
+     * 
+     * @param  color {String} color to draw
+     * @param  x {Number} x coordinate 
+     * @param  y {Number} y coordinate
+     * @param  width {Number} width of the line to draw, in pixels
+     * @param  context {CanvasRenderingContext2D} context of the canvas target
+     */
     renderLine_ : function (color, x, y, width, context) {
-      if (color != Constants.TRANSPARENT_COLOR) {
+      if (color === Constants.TRANSPARENT_COLOR || color === null) {
-        context.fillStyle = color;
+        return;
-        context.fillRect(x, y, 1, width);
       }
+      context.fillStyle = color;
+      context.fillRect(x, y, 1, width);
     },
 
     merge : function (frames) {

src/js/utils/PixelUtils.js

@@ -128,7 +128,7 @@
           var nextCol = currentItem.col + dx[i];
           var nextRow = currentItem.row + dy[i];
           try {
-            if (frame.containsPixel(nextCol, nextRow)  && frame.getPixel(nextCol, nextRow) == targetColor) {
+            if (frame.containsPixel(nextCol, nextRow) && frame.getPixel(nextCol, nextRow) == targetColor) {
               queue.push({'col': nextCol, 'row': nextRow});
             }
           } catch (e) {

test/js/testutils/TestUtils.js

@@ -1,6 +1,28 @@
 (function () {
   var ns = $.namespace('test.testutils');
 
+  /**
+   * Frame.createFromGrid accepts grids that are rotated by 90deg from
+   * the visual/usual way. (column-based grid)
+   *
+   * For testing, it's easier for be able to specify a row-based grid, because
+   * it visually matches what the image will look like.
+   *
+   * For instance :
+   *
+   * [[black, black, black],
+   *  [white, white, white]]
+   *
+   * we expect this to be a 3x2 image, one black line above a white line.
+   *
+   * However Frame.createFromGrid needs the following input to create such an image :
+   * 
+   * [[black, white],
+   *  [black, white],
+   *  [black, white]] 
+   *
+   * This helper will build the second array from the first array.
+   */
   ns.toFrameGrid = function (normalGrid) {
     var frameGrid = [];
     var w = normalGrid[0].length;

test/js/utils/FrameUtilsTest.js

@@ -3,35 +3,41 @@ describe("FrameUtils suite", function() {
   var red = '#ff0000';
   var transparent = Constants.TRANSPARENT_COLOR;
 
+  // shortcuts
+  var toFrameGrid = test.testutils.toFrameGrid;
+  var frameEqualsGrid = test.testutils.frameEqualsGrid;
+
   it("merges 2 frames", function () {
+    var B = black, R = red, T = transparent;
     var frame1 = pskl.model.Frame.fromPixelGrid([
-      [black, transparent],
+      [B, T],
-      [transparent, black]
+      [T, B]
     ]);
 
     var frame2 = pskl.model.Frame.fromPixelGrid([
-      [transparent, red],
+      [T, R],
-      [red, transparent]
+      [R, T]
     ]);
 
     var mergedFrame = pskl.utils.FrameUtils.merge([frame1, frame2]);
-    expect(mergedFrame.getPixel(0,0)).toBe(black);
+    frameEqualsGrid(mergedFrame, [
-    expect(mergedFrame.getPixel(0,1)).toBe(red);
+      [B, R],
-    expect(mergedFrame.getPixel(1,0)).toBe(red);
+      [R, B]
-    expect(mergedFrame.getPixel(1,1)).toBe(black);
+    ]);
   });
 
   it("returns same frame when merging single frame", function () {
-    var frame1 = pskl.model.Frame.fromPixelGrid([
+    var B = black, T = transparent;
-      [black, transparent],
+    var frame1 = pskl.model.Frame.fromPixelGrid(toFrameGrid([
-      [transparent, black]
+      [B, T],
-    ]);
+      [B, T]
+    ]));
 
     var mergedFrame = pskl.utils.FrameUtils.merge([frame1]);
-    expect(mergedFrame.getPixel(0,0)).toBe(black);
+    frameEqualsGrid(mergedFrame, [
-    expect(mergedFrame.getPixel(0,1)).toBe(transparent);
+      [B, T],
-    expect(mergedFrame.getPixel(1,0)).toBe(transparent);
+      [B, T]
-    expect(mergedFrame.getPixel(1,1)).toBe(black);
+    ]);
   });
 
   var checkPixelsColor = function (frame, pixels, color) {
@@ -42,9 +48,10 @@ describe("FrameUtils suite", function() {
   };
 
   it ("converts an image to a frame", function () {
+    var B = black, T = transparent;
     var frame1 = pskl.model.Frame.fromPixelGrid([
-      [black, transparent],
+      [B, T],
-      [transparent, black]
+      [T, B]
     ]);
 
     var image = pskl.utils.FrameUtils.toImage(frame1);
@@ -57,48 +64,66 @@ describe("FrameUtils suite", function() {
 
     var biggerFrame = pskl.utils.FrameUtils.createFromImage(biggerImage);
 
-    checkPixelsColor(biggerFrame, [
+    frameEqualsGrid(biggerFrame, [
-      [0,0],[0,1],[0,2],
+      [B, B, B, T, T, T],
-      [1,0],[1,1],[1,2],
+      [B, B, B, T, T, T],
-      [2,0],[2,1],[2,2],
+      [B, B, B, T, T, T],
-      [3,3],[3,4],[3,5],
+      [T, T, T, B, B, B],
-      [4,3],[4,4],[4,5],
+      [T, T, T, B, B, B],
-      [5,3],[5,4],[5,5]
+      [T, T, T, B, B, B]
-    ], black);
+    ]);
-
-    checkPixelsColor(biggerFrame, [
-      [0,3],[0,4],[0,5],
-      [1,3],[1,4],[1,5],
-      [2,3],[2,4],[2,5],
-      [3,0],[3,1],[3,2],
-      [4,0],[4,1],[4,2],
-      [5,0],[5,1],[5,2]
-    ], transparent);
   });
 
   it ("[LayerUtils] creates a layer from a simple spritesheet", function () {
-    var frame = pskl.model.Frame.fromPixelGrid([
+    var B = black, R = red;
-      [black, red],
+
-      [red, black],
+    // original image in 4x2
-      [black, black],
+    var frame = pskl.model.Frame.fromPixelGrid(toFrameGrid([
-      [red, red]
+      [B, R, B, R],
-    ]);
+      [R, B, B, R]
+    ]));
+
     var spritesheet = pskl.utils.FrameUtils.toImage(frame);
 
+    // split the spritesheet by 4
     var frames = pskl.utils.LayerUtils.createLayerFromSpritesheet(spritesheet, 4);
+
+    // expect 4 frames of 1x2
     expect(frames.length).toBe(4);
 
-    expect(frames[0].getPixel(0,0)).toBe(black);
+    // verify frame content
-    expect(frames[0].getPixel(0,1)).toBe(red);
+    frameEqualsGrid(frames[0], [
+      [B],
+      [R]
+    ]);
+    frameEqualsGrid(frames[1], [
+      [R],
+      [B]
+    ]);
+    frameEqualsGrid(frames[2], [
+      [B],
+      [B]
+    ]);
+    frameEqualsGrid(frames[3], [
+      [R],
+      [R]
+    ]);
+  });
 
-    expect(frames[1].getPixel(0,0)).toBe(red);
+  it ("supports null values in frame array", function () {
-    expect(frames[1].getPixel(0,1)).toBe(black);
+    var B = black, T = transparent;
+    var frame = pskl.model.Frame.fromPixelGrid([
+      [B, null],
+      [null, B]
+    ]);
 
-    expect(frames[2].getPixel(0,0)).toBe(black);
+    var image = pskl.utils.FrameUtils.toImage(frame);
-    expect(frames[2].getPixel(0,1)).toBe(black);
-
-    expect(frames[3].getPixel(0,0)).toBe(red);
-    expect(frames[3].getPixel(0,1)).toBe(red);
 
+    // transform back to frame for ease of testing
+    var testFrame = pskl.utils.FrameUtils.createFromImage(image);
+    frameEqualsGrid(testFrame, [
+      [B, T],
+      [T, B]
+    ]);
   });
 });