From 804a0335b14a5e46059d6e4793f4e7d0c5167862 Mon Sep 17 00:00:00 2001
From: unsettledgames <47360416+unsettledgames@users.noreply.github.com>
Date: Thu, 22 Jul 2021 19:26:40 +0200
Subject: [PATCH] Removed replaceAllOfColor, made another commenting round

---
 js/_fill.js              |  1 +
 js/_layer.js             | 58 ++++++++++++++++++++++++++++------------
 js/_line.js              |  1 +
 js/_loadPalette.js       |  1 +
 js/_mouseEvents.js       |  2 +-
 js/_move.js              |  1 +
 js/_pixelGrid.js         |  2 ++
 js/_rectSelect.js        |  1 +
 js/_rectangle.js         |  2 ++
 js/_replaceAllOfColor.js |  0
 js/_resizeCanvas.js      |  2 ++
 js/_resizeSprite.js      |  2 ++
 js/_settings.js          |  1 +
 js/_toolButtons.js       |  1 +
 js/_tools.js             |  1 +
 15 files changed, 58 insertions(+), 18 deletions(-)
 delete mode 100644 js/_replaceAllOfColor.js

diff --git a/js/_fill.js b/js/_fill.js
index b39eb14..bac646c 100644
--- a/js/_fill.js
+++ b/js/_fill.js
@@ -1,3 +1,4 @@
+// REFACTOR: fill tool onMouseDown
 function fill(cursorLocation) {
 
     //changes a pixels color
diff --git a/js/_layer.js b/js/_layer.js
index cb78224..f7ea4e5 100644
--- a/js/_layer.js
+++ b/js/_layer.js
@@ -2,25 +2,36 @@
 let layerList;
 // A single layer entry (used as a prototype to create the new ones)
 let layerListEntry;
-// NEXTPULL: remove the drag n drop system and use Sortable.js instead
-let layerDragSource = null;
 
 // Number of layers at the beginning
+// REFACTOR: keep in layer class, it's only used here
 let layerCount = 1;
+
 // Current max z index (so that I know which z-index to assign to new layers)
+// REFACTOR: keep in layer class, it's only used here
 let maxZIndex = 3;
 
-// When a layer is deleted, its id is added to this array and can be reused
+// When a layer is deleted, its id is added to this array and can be reus
+// REFACTOR: keep in layer class, it's only used here
 let unusedIDs = [];
+
 // Id for the next added layer
+// REFACTOR: keep in layer class, it's only used here
 let currentID = layerCount;
+
 // Layer menu
+// REFACTOR: keep in layer class, it's only used here
 let layerOptions = document.getElementById("layer-properties-menu");
+
 // Is the user currently renaming a layer?
+// REFACTOR: this one's tricky, might be part of EditorState
 let isRenamingLayer = false;
+
 // I need to save this, trust me
+// REFACTOR: keep in layer class, it's only used here and investigate about why I need to save it
 let oldLayerName = null;
 
+// REFACTOR: keep in layer class, it's only used here
 let dragStartLayer;
 
 // Binding the add layer button to the function
@@ -30,12 +41,16 @@ Events.on('click',"add-layer-button", addLayer, false);
  *
  * @param width Canvas width
  * @param height Canvas height
- * @param canvas HTML canvas element
+ * @param canvas HTML canvas element or the ID of the canvas related to the layer
  */
 class Layer {
     constructor(width, height, canvas, menuEntry) {
+        // REFACTOR: this could just be an attribute of a Canvas class
         this.canvasSize = [width, height];
+        // REFACTOR: the canvas should actually be a Canvas instance
         this.canvas = Util.getElement(canvas);
+        // REFACOTR: the context could be an attribute of the canvas class, but it's a bit easier
+        // to just type layer.context, we should discuss this
         this.context = this.canvas.getContext('2d');
         this.isSelected = false;
         this.isVisible = true;
@@ -43,7 +58,6 @@ class Layer {
         this.menuEntry = menuEntry;
 
         let id = unusedIDs.pop();
-        console.log("id creato: " + id);
 
         if (id == null) {
             id = currentID;
@@ -299,6 +313,7 @@ class Layer {
     }
 }
 
+// REFACTOR: this should probably be a method of the LayerMenu class as it involves more than 1 layer
 function flatten(onlyVisible) {
     if (!onlyVisible) {
         // Selecting the first layer
@@ -353,6 +368,7 @@ function flatten(onlyVisible) {
     }
 }
 
+// REFACTOR: this should probably be a method of the LayerMenu class as it involves more than 1 layer
 function merge(saveHistory = true) {
     // Saving the layer that should be merged
     let toMerge = currentLayer;
@@ -383,6 +399,7 @@ function merge(saveHistory = true) {
     }
 }
 
+// REFACTOR: this should probably be a method of the LayerMenu class as it involves more than 1 layer
 function deleteLayer(saveHistory = true) {
     // Cannot delete all the layers
     if (layers.length != 4) {
@@ -417,7 +434,18 @@ function deleteLayer(saveHistory = true) {
     currentLayer.closeOptionsMenu();
 }
 
+// REFACTOR: this should probably be a method of the LayerMenu class as it involves more than 1 layer
 function duplicateLayer(event, saveHistory = true) {
+    function getMenuEntryIndex(list, entry) {
+        for (let i=0; i<list.length; i++) {
+            if (list[i] === entry) {
+                return i;
+            }
+        }
+    
+        return -1;
+    }
+
     let layerIndex = layers.indexOf(currentLayer);
     let toDuplicate = currentLayer;
     let menuEntries = layerList.children;
@@ -466,6 +494,7 @@ function duplicateLayer(event, saveHistory = true) {
     }
 }
 
+// REFACTOR: why isn't this static?
 function renameLayer(event) {
     let p = currentLayer.menuEntry.getElementsByTagName("p")[0];
 
@@ -480,16 +509,7 @@ function renameLayer(event) {
     isRenamingLayer = true;
 }
 
-function getMenuEntryIndex(list, entry) {
-    for (let i=0; i<list.length; i++) {
-        if (list[i] === entry) {
-            return i;
-        }
-    }
-
-    return -1;
-}
-
+// REFACTOR: put in LayerMenu class as it involves more than 1 layer
 // Finds a layer given its name
 function getLayerByName(name) {
     for (let i=0; i<layers.length; i++) {
@@ -503,6 +523,7 @@ function getLayerByName(name) {
     return null;
 }
 
+// REFACTOR: put in LayerMenu class as it involves more than 1 layer
 // Finds a layer given its id
 function getLayerByID(id) {
     for (let i=0; i<layers.length; i++) {
@@ -516,6 +537,7 @@ function getLayerByID(id) {
     return null;
 }
 
+// REFACTOR: put in LayerMenu class as it involves more than 1 layer
 function addLayer(id, saveHistory = true) {
     // layers.length - 3
     let index = layers.length - 3;
@@ -559,6 +581,7 @@ function addLayer(id, saveHistory = true) {
     return newLayer;
 }
 
+// REFACTOR: LayerMenu
 /** Saves the layer that is being moved when the dragging starts
  * 
  * @param {*} event 
@@ -567,6 +590,7 @@ function layerDragStart(event) {
     dragStartLayer = getLayerByID(layerList.children[event.oldIndex].id);
 }
 
+// REFACTOR: LayerMenu
 /** Sets the z indexes of the layers when the user drops the layer in the menu
  * 
  * @param {*} event 
@@ -595,6 +619,7 @@ function layerDragDrop(event) {
 }
 
 
+// REFACTOR: LayerMenu
 /** Merges topLayer onto belowLayer
  * 
  * @param {*} belowLayer The layer on the bottom of the layer stack
@@ -630,9 +655,8 @@ function mergeLayers(belowLayer, topLayer) {
     belowLayer.putImageData(toMergeImageData, 0, 0);
 }
 
-
+// REFACTOR: LayerMenu
 layerList = document.getElementById("layers-menu");
-
 // Making the layers list sortable
 new Sortable(document.getElementById("layers-menu"), {
     animation: 100,
diff --git a/js/_line.js b/js/_line.js
index 2e150de..3757427 100644
--- a/js/_line.js
+++ b/js/_line.js
@@ -1,3 +1,4 @@
+// LINE TOOL
 function diagLine(lastMouseClickPos, zoom, cursorLocation) {
 
     let x0 = Math.floor(lastMouseClickPos[0]/zoom);
diff --git a/js/_loadPalette.js b/js/_loadPalette.js
index e7534e1..2003f34 100644
--- a/js/_loadPalette.js
+++ b/js/_loadPalette.js
@@ -1,5 +1,6 @@
 //this is called when a user picks a file after selecting "load palette" from the new pixel dialogue
 
+// REFACTOR: ColorModule? Or maybe move it to _palettes and give a name to that IIFE
 // TODO: load palette from .lpe file
 document.getElementById('load-palette-browse-holder').addEventListener('change', function () {
     if (this.files && this.files[0]) {
diff --git a/js/_mouseEvents.js b/js/_mouseEvents.js
index 0577caa..3e566fe 100644
--- a/js/_mouseEvents.js
+++ b/js/_mouseEvents.js
@@ -1,6 +1,6 @@
 // REFACTOR: let's keep this one global for now, unfortunately it's used by just some tools to keep track of 
 // their state: I'd wait after we refactor the tools themselves before removing this variable, which should
-// ideally be updated in Input.js
+// ideally be updated in Input.js what a mess what a mess what a mess what a mess
 var lastMouseMovePos;
 
 //mousedown - start drawing
diff --git a/js/_move.js b/js/_move.js
index 82165ae..7226424 100644
--- a/js/_move.js
+++ b/js/_move.js
@@ -1,3 +1,4 @@
+// REFACTOR: let's keep this one for the end
 var imageDataToMove;
 var originalDataPosition;
 var canMoveSelection = false;
diff --git a/js/_pixelGrid.js b/js/_pixelGrid.js
index 775327c..9508dbc 100644
--- a/js/_pixelGrid.js
+++ b/js/_pixelGrid.js
@@ -1,3 +1,5 @@
+// REFACTOR: inherit from Layer, override init method (call super as well)
+
 // Start colour of the pixel grid (can be changed in the preferences)
 let pixelGridColor = "#000000";
 // Distance between one line and another in HTML pixels
diff --git a/js/_rectSelect.js b/js/_rectSelect.js
index 1e78e62..f49095e 100644
--- a/js/_rectSelect.js
+++ b/js/_rectSelect.js
@@ -1,3 +1,4 @@
+// RECT SELECTION TOOL
 var isRectSelecting = false;
 let startX;
 let startY;
diff --git a/js/_rectangle.js b/js/_rectangle.js
index fee4a8c..4c13e17 100644
--- a/js/_rectangle.js
+++ b/js/_rectangle.js
@@ -1,3 +1,5 @@
+// RECT TOOL
+
 // Saving the empty rect svg
 var emptyRectangleSVG = document.getElementById("rectangle-empty-button-svg");
 // and the full rect svg so that I can change them when the user changes rect modes
diff --git a/js/_replaceAllOfColor.js b/js/_replaceAllOfColor.js
deleted file mode 100644
index e69de29..0000000
diff --git a/js/_resizeCanvas.js b/js/_resizeCanvas.js
index f0cc1c3..806e84d 100644
--- a/js/_resizeCanvas.js
+++ b/js/_resizeCanvas.js
@@ -1,3 +1,5 @@
+// REFACTOR: method of File class probably
+
 /* This scripts contains all the code used to handle the canvas resizing */
 
 // Resize canvas pop up window
diff --git a/js/_resizeSprite.js b/js/_resizeSprite.js
index 9744bd6..402fd9a 100644
--- a/js/_resizeSprite.js
+++ b/js/_resizeSprite.js
@@ -1,3 +1,5 @@
+// REFACTOR: method of File class probably
+
 /* This scripts contains all the code used to handle the sprite scaling */
 // Should I keep the sprite ratio?
 let keepRatio = true;
diff --git a/js/_settings.js b/js/_settings.js
index 0ac1009..137e77a 100644
--- a/js/_settings.js
+++ b/js/_settings.js
@@ -1,3 +1,4 @@
+// REFACTOR: convert to IIFE
 var settings;
 
 if (!Cookies.enabled) {
diff --git a/js/_toolButtons.js b/js/_toolButtons.js
index 9d4c73c..e81bae0 100644
--- a/js/_toolButtons.js
+++ b/js/_toolButtons.js
@@ -1,3 +1,4 @@
+// REFACTOR: add to single Tool implementations
 //pencil
 Events.on('click',"pencil-button", function(){
     tool.pencil.switchTo();
diff --git a/js/_tools.js b/js/_tools.js
index b443ed8..15b6fa6 100644
--- a/js/_tools.js
+++ b/js/_tools.js
@@ -1,3 +1,4 @@
+// REFACTOR: this is a nice base for the Tool class
 //tools container / list, automatically managed when you create a new Tool();
 var tool = {};