2
\$\begingroup\$

I am looking for some advice on some speed increases on my Adobe Illustrator (AI) script. Just so you know, the script runs fine and is at a decent speed (couple seconds to maybe a minute max for large files), but would like to speed it up further.

A couple things to note about how AI and ExtendScript(ES) (my IDE) works (that I have observed):

  1. Using layer or layers as a variable seems to cause some issues (have not determined an exact cause but there were somethings not working when I was using it, I think it may be a reserved word) so instead I use 'currentLayer' or 'allLayers' instead.

  2. I don't have access to many common javascript libraries (no indexOf or forEach).

  3. For my recursive loop, I have tried two different ways to run it that would make it cleaner or faster, neither have worked. So not much hope of improvements there (it works fine so far).

Script description: This is a simple, yet complex script. Its primary purpose is to search the layers in a adobe illustrator file and find the match (of the inputted text). There is also options for exact search (exact string), or not, case sensitive search, or find without these specifications. I also have a sorting script that sorts the layers (not included) if anyone suggests a search that requires a sorted list.

Here is a document that has a lot (almost all) of the functions I can use with AI - Illustrator-Scripting-Reference-JavaScript.pdf

#target illustrator

if (app.documents.length > 0) {
    var doc = app.activeDocument;
    var docLayers = doc.layers;

    var inputText;
    var caseSense = false;
    var exactWord = false;
    var match = false;
    var textSearch = false;

    var allLayers = [];
    var layerNames = [];

    // Display GUI
    startGUI();

    // Main Search function
    function startSearch(layers) {
        match = false;
        flattenLayersRecursive(layers);

        searchLayers();

        // When match is found, show dialog
        if (match) {
            alert("Found match!");
            match = false;
        } else {
            alert("No match found.");
            match = false;
        }
    }


    // Recursive loop to search all layers in active document
    function flattenLayersRecursive ( layers ) {
        var length = layers.length;
        var currentLayer ;

        try {
            for (var i = length; i--;) { //var i = length; i--; //var i = 0; i <= length; i++
                currentLayer = layers[i];

                var locked = currentLayer.locked;
                var visible = currentLayer.visible;

                if (visible == true || locked == false) {
                    checkLayerType(inputText, currentLayer);

                    // Search for sublayers, page items or group items
                    if (currentLayer.layers) {
                        flattenLayersRecursive(currentLayer.layers);                            
                        flattenLayersRecursive(currentLayer.groupItems);
                        flattenLayersRecursive(currentLayer.pathItems);
                        flattenLayersRecursive(currentLayer.compoundPathItems);
                        flattenLayersRecursive(currentLayer.symbolItems);
                        flattenLayersRecursive(currentLayer.textFrames);
                    }
                }
            }
        } catch (error) {
            logger (error);
        }
    }// end flattenLayersRecursive

    // Search for match between input and layer name
    function checkLayerType( currentLayer ) {
        try {
            if (inputText) {
                var layerName = "";
                var layerType = currentLayer.typename;
                var searchIndex = -1;
                var exact = false;

                switch (layerType) {
                    default:
                        layerName = currentLayer.name;
                        searchIndex = findLayerName(inputText, layerName);

                        selectLayer(searchIndex, currentLayer, layerType) 
                        break;
                    case "SymbolItem":
                        layerName = currentLayer.symbol.name;
                        searchIndex = findLayerName(inputText, layerName);

                        selectLayer(searchIndex, currentLayer, layerType) 
                        break;
                    case "TextFrame":
                        layerName = currentLayer.contents;
                        searchIndex = findLayerName(inputText, layerName);

                        selectLayer(searchIndex, currentLayer, layerType) 
                        break;
                    case "Layer":
                        layerName = currentLayer.name;
                        searchIndex = findLayerName(inputText, layerName);

                        selectLayer(searchIndex, currentLayer, layerType) 
                        break;
                } // end Switch


            } // end inputText
        } catch (error) {
            logger(error);
        }
    } // end Search

    function findLayerName(inputText, layerName) {
        var searchIndex = -1;
        var caseSensitive = caseSense;
        var exact = exactWord;

        if (caseSensitive) {
            searchIndex = layerName.indexOf(inputText) ;
        } else if (exact) {                 
            if (layerName === inputText) {
                    searchIndex = 0;
            }
        } else {
            searchIndex = layerName.toLowerCase().indexOf(inputText.toLowerCase());
        }

        return searchIndex;
    }

    function selectLayer(searchIndex, currentLayer, layerType) {

        if ( searchIndex != -1 ) {
            if (layerType != "Layer") {
                currentLayer.selected = true;
                match = true;
            } else if (layerType == "Layer") {
                currentLayer.hasSelectedArtwork = true;
                //layer.selected = true;
                match = true;
            }
        }
    } // end selectLayer

    // Display GUI
    function startGUI() {

        // Create Main Window
        var win = new Window( "dialog", "Layer Search Selection", undefined );

        // Enable use of 'Enter' key
        win.addEventListener ("keydown", function(kd) {enter(kd) });

        // Style for Main Window
        win.orientation = "column";
        win.alignChildren = ["fill", "fill"];
        //win.preferredSize = [150, 350];

        // Style for Search group
        var searchGrp = win.add("panel", undefined, "Search Layers");
        searchGrp.orientation = "column";
        searchGrp.alignChildren = ["fill", "fill"];

        var titleMsg = searchGrp.add ("statictext", undefined, "Layer name to search:");
        var txt_Input = searchGrp.add("edittext { characters: 1, justify: 'center', active: true }");
        txt_Input.helpTip = "Input letters to search";

        // Search Button
        var searchBtn = searchGrp.add("button", undefined, "Search");
        searchBtn.helpTip = "Search from text items";
        searchBtn.onClick = function() {
            inputText = txt_Input.text;

            startSearch(docLayers);
            app.redraw();
        }

        // Use Enter key
        function enter(k) {
            if (k.keyName == "Enter") {
                inputText = txt_Input.text;

                startSearch(docLayers);
                app.redraw();
            }
        }

        // Listener for the input
        txt_Input.onChanging = function() {
            app.redraw();
        }

        // Options
        var optionsGrp = win.add("panel", undefined, "Options");
        optionsGrp.orientation = "row";
        optionsGrp.margins = [10, 15, 10, 6];
        optionsGrp.alignChildren = ["fill", "fill"];

        // Radio button: Case Sensitive
        var rdb_caseSensitive = optionsGrp.add ("radiobutton", undefined, "Match Case");
        rdb_caseSensitive.helpTip = "Case sensitive search";
        rdb_caseSensitive.value = false;

        // Listener: Case Sensitive
        rdb_caseSensitive.onClick = function() {
            caseSense = rdb_caseSensitive.value;
            app.redraw();
        }

        // Radio button: Exact Word
        var rdb_exactWord = optionsGrp.add ("radiobutton", undefined, "Match Exact Word");
        rdb_exactWord.helpTip = "Search exact word";
        rdb_exactWord.value = false;

        // Listener: Exact Word
        rdb_exactWord.onClick = function() {
            exactWord = rdb_exactWord.value;
            app.redraw();
        }

        // Radio button: None
        var chk_none = optionsGrp.add ("radiobutton", undefined, "None");
        chk_none.helpTip = "Use no extra option";
        chk_none.value = false;

        // Radio button: None
        chk_none.onClick = function() {
            exactWord = false;
            caseSense = false;
            app.redraw();
        }

        // Style for Extra
        var extraGrp = win.add("panel", undefined, "Extra");
        extraGrp.orientation = "row";
        extraGrp.alignChildren = ["fill", "fill"];

        // Button: Deselect all
        var btnDeselect = extraGrp.add('button', undefined, "Deselect All");
        btnDeselect.helpTip = "Deselect All Objects";
        btnDeselect.onClick = function () { 
            app.executeMenuCommand ('deselectall'); 
            app.redraw(); 
        };

        // Close button
        var quitBtn = win.add("button", undefined, "Close");
        quitBtn.helpTip = "Press Esc to Close";

        // Event listener for the quit button
        quitBtn.onClick = function() {   
            win.close();   
        }  

        // Centering & Show Window
        win.center();
        win.show(); 
    }// end startGUI
} else {
        alert("You do not have any document opened!");
}
\$\endgroup\$
2
\$\begingroup\$

Some thoughts (it's quite a large script, so these are first impressions, and I skipped over all the GUI code):

  • It doesn't look like your script stops once it's found something. I can't tell if the idea is to select all matching layers, or simply the first matching layer. You description seems to imply the latter ("find the match", emphasis added), but your code seems to select every match. It also pointlessly recurses into layers that already match; if the layer's selected, you should be able to skip its children.

    So if the idea is to only find the first match, and you want your script to run faster, I'd suggest, well, stopping after the first match...

  • You seem to always create a local variable for something you already have access to. This is really quite unnecessary, and in some ways actually make things harder to read and follow. For instance:

    var locked = currentLayer.locked;
    var visible = currentLayer.visible;
    
    if (visible == true || locked == false) {
        // ...
    

    Why not just use currentLayer.locked directly? I say it makes things harder to follow because context is lost - in this case the context is currentLayer.. The above is simple, and doesn't cause trouble, but in other cases, I had to look back and forth a few times to figure out what a local variable actually was. Sometimes it's useful to make local variables as shortcuts, but here it's more like adding a layer of obfuscation.

  • The global variables irk me. You're dumping state into the global scope, and relying on code to both read and write from it. But the core of the code can be handled functionally, and again, your way is trickier to understand. For instance:

    function startSearch(layers) {
         match = false;
         flattenLayersRecursive(layers);
    
         searchLayers();
    
         if (match) {
             // ...
    

    At first I didn't expect side-effects from the function calls, so if match is set to false it should still be false when you hit the if statement. But secretly, something messes with match - i.e. side-effects.

    At first I thought it was searchLayers but that isn't defined anywhere (!?) so I have no idea what it does. It has no arguments and no output, so... what's going on?

    As it turns out, it's flattenLayersRecursive that does the searching. It takes some layers and returns... nothing? Now I'm really confused.

    Sometimes side-effects are just the easiest way to do things, but even easier and cleaner would be to simply feed in the layers and query, and return the answer:

    var match = search(layers, query); // search these layers, and give me an answer
    
  • Structure. In sort of a similar vein to the reliance on side-effects, you have some slightly odd structure. In checkLayerType, you always call selectLayer - and then it has to decide if it should actually select anything. Why not only call it when necessary? I.e. when you truly want to select a layer.

  • Repetition. Your switch statement in checkLayerType does the same for all its branches, except for the value of layerName. Everything else can go after the switch, when the layer name has been determined. By the way, the case for "Layer" does the same as the default case, and so can be omitted. (Also, it's convention to put the default branch at the end - not the beginning.)

  • Naming. checkLayerType doesn't really "check a layer's type" - it does the per-layer name matching. flattenLayersRecursive doesn't flatten anything (but it is recursive, I'll give you that). It's really the do-the-actual-search function. Given its name I'd expect it to return a flat array of layers, though. And findLayerName doesn't find a layer by name, or find a layer's name: It looks for a string in a layer's name. And so on. Name things for what they actually do.

    Of course your functions are doing way too much right now, and it's all side-effects, so the accurate name of recursivelyWalkTheLayerHieararchyAndSendEverythingToThreeOtherFunctionsThatMightChangeAGlobalVariable is a nonstarter. A well-named function does what it says on the tin - nothing more, nothing less. And conversely, a function that does a specific task is easy to name well.

Here's an attempt at rewriting some of it (again, skipping the GUI code). Not guaranteeing it'll work in Illustrator's strange JS runtime, but it illustrates (no pun intended, I swear) some of the points above. It's not at all perfect, but it's better.

Incidentally, I suspect the only reason you can't use layer as a variable name, is because you're trying to use it as a global variable, and thus overwriting existing one. Local variables, however, should be fine, so inside functions you can probably use the name layer.

// a "constant" (insofar as old-school JS has constants)
var CHILD_LAYERS = ['layers', 'groupItems', 'pathItems', 'compoundPathItems', 'symbolItems', 'textFrames'];

// get a layer's 'searchable name'
function getLayerName(layer) {
  if (layer.typename === 'SymbolItem') {
    return layer.symbol.name;
  }

  if (layer.typename === 'TextFrame') {
    // this is a little iffy - here it's not actually returning the layer's name
    // but the contents of a text frame. Don't know if I like that, since it goes
    // against the function's stated purpose; to get the layer's *name*.
    return layer.contents;
  }

  return layer.name;
}

// match a given layer against the query
function matchLayer(layer, query, caseSensitive, wholeString) {
  var name = getLayerName(layer);

  // note: Your original code treated case-sensitivity and
  // exact matching as mutually exclusive. This treats them
  // as complementary; you can match the 'whole name' but
  // do so in a case-insensitive manner.

  if (!caseSensitive) {
    name = name.toLowerCase();
  }

  if (wholeString) {
    return name === query;
  } else {
    return name.indexOf(query) !== -1;
  }
}

// get all the searchable children of a given layer as an array
function getChildLayers(layer) {
  var children = [];

  for(var i = 0, l = CHILD_LAYERS.length ; i < l ; i++) {
    children.push.apply(children, layer[CHILD_LAYERS[i]] || []);
  }

  return children;
}

// selects the given layer
function selectLayer(layer) {
  if (layer.typename === 'Layer') {
    layer.selected = true;
  } else {
    layer.hasSelectedArtwork = true;
  }
}

// the recursive find function
function findLayer(layers, query, caseSensitive, wholeString) {
  for(var i = 0, l = layers.length ; i < l ; i++) {
    var layer = layers[i];

    if(!layer.visible && layer.locked) {
      continue; // skip to next iteration of the loop
    }

    // try to match the layer
    if (matchLayer(layer, query, caseSensitive, wholeString)) {
      return layer; // we found a matching layer, so stop and return it
    }

    // look for nested matches
    var match = findLayer(getChildLayers(layer), query, caseSensitive, wholeString);
    if (match) {
      return match;
    }
  }

  return null; // never found anything
}

// usage example of sorts
function searchLayers(query, caseSensitive, wholeString) {
  // this bit of code could go somewhere else; the important thing
  // is just to lowercase the query *once*; not every time we have
  // to check it against a layer
  if (!caseSensitive) {
    query = query.toLowerCase();
  }

  // look for a match
  var match = findLayer(app.activeDocument.layers, query, caseSensitive, wholeString);

  if (match) {
    selectLayer(match);
    alert('Found a match');
  } else {
    alert('No matches found');
  } 
}

edit: Here's a version that returns an array of all matching layers. It still skips over the children already-matched layers, though:

function findLayers(layers, query, caseSensitive, wholeString) {
  var matches = [];

  for(var i = 0, l = layers.length ; i < l ; i++) {
    var layer = layers[i];

    if(!layer.visible && layer.locked) {
      continue;
    }

    if (matchLayer(layer, query, caseSensitive, wholeString)) {
      matches.push(layer);
    } else {
      matches = matches.concat(findLayers(getChildLayers(layer), query, caseSensitive, wholeString));
    }
  }

  return matches;
}

function searchLayers(query, caseSensitive, wholeString) {
  if (!caseSensitive) {
    query = query.toLowerCase();
  }

  var matches = findLayers(app.activeDocument.layers, query, caseSensitive, wholeString);

  if (!matches.length) {
    alert('No matches found');
    return;
  }

  for(var i = 0, l = matches.length ; i < l ; i++) {
    selectLayer(matches[i]);
  }

  alert('Found ' + matches.length + ' ' + (matches.length === 1 ? 'match' : 'matches'));
}

One thing that stood out is the logic for skipping a layer: If a layer is hidden and locked, then it's skipped. That seems a little odd. Why should it have to be both? Shouldn't that be hidden or locked?

\$\endgroup\$
  • \$\begingroup\$ Ok, let me clear a few things I didn't mention. The script is meant to search all possible matches, not the first match (I did have this but needed it to do the latter). Second, match is used in the selectLayer and it used to determine if a match is found or not (it works). Also flattenLayersRecursive isn't meant to return anything it just loops through all the layers. The currentLayer.selected = true selects the layers when found. Also, I'm really bad at making names for classes, a few I use were recommended on another post. Also I used layer(s) as a local var not global. \$\endgroup\$ – Lloyd Smith Feb 4 '17 at 3:47
  • \$\begingroup\$ Also the reason I use a lot of local variables (even when unnecessary) is because I read this article about optimization techniques. Something to know, my code is looping over 20,000 times (layers) so I need as much optimization as possible. javaworld.com/article/2077647/build-ci-sdlc/… \$\endgroup\$ – Lloyd Smith Feb 4 '17 at 3:48
  • \$\begingroup\$ @LloydSmith "flattenLayersRecursive isn't meant to return anything it just loops through all the layers". But it should return something, is what I'm saying. I'm sure the code does what it's meant to do – I just don't think it's the right thing to do. And a Java optimization trick may have no, or directly adverse, effects in JavaScript (the two languages have nothing in common). I'd advice you to thoroughly benchmark it. Premature optimization is the root of all evil, etc.. As for multiple results: Alright, well, that just wasn't clear. \$\endgroup\$ – Flambino Feb 4 '17 at 4:17
  • \$\begingroup\$ @LloydSmith (Also that post on optimization will be 20 years old in a few months - I'm not sure those tricks work in Java itself anymore) \$\endgroup\$ – Flambino Feb 4 '17 at 4:22
  • \$\begingroup\$ Ah sorry about that, I thought I used another one that covered similar topics, but couldn't find it. Those tricks still helped me speed it up a bit. Another thing, for locked/hidden it should only loop over visible and unlocked layers, if they are locked/hidden or both it should be ignored, no matter what. Also, how would I benchmark in javascript? I still don't understand why premature optimization is bad, is it wrong to want to improve a program and make it run faster? \$\endgroup\$ – Lloyd Smith Feb 8 '17 at 14:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.