Hi Marc,

I wouldn't infer any such thing. Looking at your code, it probably works fine.

Chances are, my code was probably easier to read...

Hey, Marc: I'm going to be sort of blunt. Your code is hard for most people to read because it uses Javascript idioms they are entirely unfamiliar with, which don't make a lot of sense to people not really familiar with the language. And that's what most people here are. Let's take a walk through it:

(function(/*obj|undefined*/HOST)

The comment itself is confusing. Actually, it's even confusing to me.

    HOST || (HOST=$.global);

Tbe || idiom is confusing. Most people don't know what it means offhand.

    // ---
    // Prevent the current (startup) script from being uselessly rerun
    // ---
    if( HOST[$.engineName] )
        {
        alert( "This script is automatically executed at startup. You shouldn't run it manually." );

Well, this isn't confusing, but I think it's bad. One of the important things to do is to test what happens when the menus already exist, and that's easiest to do by rerunning the script.

    var MENU_NAME = "My Test Menu",

All caps for a function-local variable? Even using all caps for a globally scoped variable (Crockford) is confusing, but for a function-scope variable? That confuses me!

        FEATURES = [
            { caption: "My Menu Item 1", fileName: 'File-One.jsx', subName: "" },

Use of Objects is justified here, but they are probably confusing.

        FEATURE_LOCATION_PATH = (function()

Anonymous functions...definitely confusing! I'm also not sure what the point of this function heere is. Why not execute the code directly

on the next line?

            try{ f=app.activeScript; }
            catch(_){ f=File(_.fileName); }

Variables named "_"? Confusing!

            })();

And this is why anonymous functions are confusing! Not much to be done about it, but why use one where you don't need to. It's not like we're polluting the global namespace here.

    while( i-- )

Use of a while loop is a totally legitimate style choice, but I think it's typically less readable than a for loop. YMMV, of course!

        t = FEATURES[i];

I'm horribly guilty of this, but single-letter variable names are not the most readable.

            (t.action = app.scriptMenuActions.add( t.caption )).
                addEventListener('onInvoke', f);

More parentheses imply more mental gymnastics to understand. Breaking this into two lines,

with t.action=addEventListener... would help readability.

  var s,
        n = FEATURES.length,
        subs = {},
        sub = null;

Wait, a mid-function var block? Gasp! What's up with that? Crockford's right on this. One var statement and all at the top.

I am not sure why we're initializing sub to null -- what's wrong with the good old undefined? This probably doesn't qualify as confusing.

    for( i=0 ; i < n ; ++i )

Caching FEATURES.length in n is good for performance, but it probably doesn't matter here, and makes it more confusing to read. Not by much, but let's say half a confusion point. I would also say that "++i" is harder to read than "i++", maybe to the tune of 0.1 confusion-points, but maybe that's just me. YMMV Extremely.

        sub = (s=t.subName) ?
            ( subs[s] || (subs[s]=mnu.submenus.add( s, LO_END )) ) :
            mnu;


The ternary conditional operator, ?:, is pretty much always hard to read. And it's worse with short-circuit or-ing, ||, or three close-parens in rapid succession. Of course ?: tends to promote extra () as it almost always becomes ()?(): so...

        }
})();

And then it's worth re-emphasizing, basically nobody understands the anonymous function paradigm .

So there we have it, that's why I think your code is confusing to most readers.

(On the other hand Dan's final code ignores my recommendation and still has those abysmal ==null tests in there. What's up with that? )

That seemed to do it... Weird...

null works perfectly well for menu actions. I'm not sure why we're making a deal of this...

FWIW, here's the IsValid function that I use:

IsValid = function (obj){
    try{
        if(!obj){return false}
        if(kAppVersion>=6){
            return obj.isValid;
        }
        var test = obj.parent;
        return true;
    }
    catch(err){return false;}
}

kAppVersion is a constant I have defined like so:

kAppVersion = parseFloat(app.version);

(Every valid object in InDesign has a parent.)

Harbs

( P.S. The test variable name is extraneous, but it makes it obvious what it's doing...)

John Hawkinson wrote:

 

By the way, there's some sample Adobe script that creates menus.

Is this what you're talking about? It's distributed in the InDesign SDK...

XHTMLExportMenuItem.install = function()
{    
    if(app.name == "Adobe InDesign") {
        // set up our script preferences
        var currDOMVersion = app.scriptPreferences.version;
        if (currDOMVersion != XHTMLExportMenuItem.requiredDOMVersion)
            app.scriptPreferences.version = XHTMLExportMenuItem.requiredDOMVersion;
 
        // determine and store the path from were we load our scripts from
        XHTMLExportMenuItem.storeScriptsFolderPath();
        // load XHTMLExporter for the strings
        var xhtmlExporterScript = XHTMLExportMenuItem.loadScript('XHTMLExport.jsxbin');
        if ( !xhtmlExporterScript.exists )
        {
            xhtmlExporterScript = XHTMLExportMenuItem.loadScript('XHTMLExport.jsx');
        }
        assert( xhtmlExporterScript.exists, "XHTMLExport.jsx* missing; load failed" ) ;
        if ( xhtmlExporterScript.exists )
        {
            var cacheCurrent = Folder.current ;
            try
            {
                Folder.current = XHTMLExportMenuItem.scriptsFolder ;
                app.doScript( xhtmlExporterScript ) ;
            }
            finally
            {
                Folder.current = cacheCurrent ;
            }
            if ($.locale != 'en_US')
            {
                // try to load localized strings
                var localizationScript = XHTMLExportMenuItem.loadScript('Resources/XHTMLStrings-' + $.locale + '.jsxbin');
                if ( !localizationScript.exists )
                {
                    localizationScript = XHTMLExportMenuItem.loadScript('Resources/XHTMLStrings-' + $.locale + '.jsx');
                }
                assert( localizationScript.exists, 'Resources/XHTMLStrings-' + $.locale + '.jsx* missing; load failed' ) ;
                if ( localizationScript.exists )
                {
                    var cacheCurrent = Folder.current ;
                    try
                    {
                        Folder.current = Folder( XHTMLExportMenuItem.scriptsFolder + '/Resources' ) ;
                        app.doScript( localizationScript ) ;
                    }
                    finally
                    {
                        Folder.current = cacheCurrent ;
                    }
                }
            }
            // menu action for the File:Export for menu
            var actionname = localize(xhtmlExportStrings.FORDREAMWEAVERACTIONNAME);
            var action = app.scriptMenuActions.item(actionname);
            if(action == null) {
                // first launch:
                // we need to create the menu action
                var action = app.scriptMenuActions.add(actionname);
                action.area = app.translateKeyString('$ID/KBSCE File menu: ExportForSubMenu');
                action.checked = false;
                action.enabled = true;
            }
 
            // install event listeners for our action
            action.addEventListener("onInvoke", XHTMLExportMenuItem.exportSelectedItems);    
            action.addEventListener("beforeDisplay", XHTMLExportMenuItem.enableDisable);
            action.addEventListener("afterInvoke", XHTMLExportMenuItem.cleanup);
 
            // install the menu item
            var exportForMenu = app.menus.item(app.translateKeyString('$ID/Main')).submenus.item(app.translateKeyString('$ID/&File')).submenus.item(app.translateKeyString('$ID/ExportForSubMenu')) ;
            exportForMenu.menuItems.add(action);
 
            // store the action in a class variable
            XHTMLExportMenuItem.action = action;
        }
        if(currDOMVersion != XHTMLExportMenuItem.requiredDOMVersion)
            app.scriptPreferences.version = currDOMVersion;
    }
}

Harbs

John Hawkinson wrote:

 

Replace if (var==null) with if (!var). === is almost always better than ==, because truthiness of == causes confusion and problems. But in ifs, you might as well use the implicit truthiness and not test explicitly, and write for clarity. (Someone is going to say this argument is defective ).

 

In my opinion, discussions about == vs. === vs implicit resolutions is kind of silly. I have no issue using ==. Just make sure you understand what it means.

Personally, I only use === when I really need it. That way it's clear what I'm testing for... When I'm testing for bull or boolean values, I usually skip the "==" altogether.

I don't take everything Crawford says as if it's the bible. They're only his opinions after all...

Harbs

Marc Autret wrote:

 

 

Because—I totally agree with John— using ==null is both a bad practice and semantically irrelevant.

 

Tell that to Jonathan Brown who designed it in the first place!

(See the code I pasted above.)

(I love making grown men cry...)

The snippet I posted above was written by Jonathan Brown. It's pretty obvious where it came from (HTML export).

The font snippet is in the InDesign Scripting Guide pdf.

I actually came across a situation where === was problematic, but I don't remember the details.

I think a common case is confusion between null and undefined. For instance:

var v;

...

if (v==0) { v="now initialized!"; }

will "work" and initialize v, because v is undefined, and "undefined==0" returns true. So if you just changed it to

var v;

...

if (v===0) { v="now initialized"; }

then v would never get initialized. Of course, this is bad code because you should have initialized v in the var line anyhow, or you should have tested it against "undefined" and not "0" or "null," or halfadozen other reasons.

Anyhow, I feel like your exception proves the rule. Use '===' always, unless you learn the language well enough to require truthiness promotion, because if you don't, you are likely to make an assumption about whether something is false versus null versus undefined, and that assumption will come back and hurt you later.

John Hawkinson wrote:

 

Anyhow, I feel like your exception proves the rule. Use '===' always, unless you learn the language well enough to require truthiness promotion, because if you don't, you are likely to make an assumption about whether something is false versus null versus undefined, and that assumption will come back and hurt you later.

This is a bit like philosophy. You can argue both sides until you're blue in the face. Just about anyone coming from a C background would probably disagree. There's quite a lot of people who look at 0 and false as synonymous...

Whatever; I have bigger fish to fry...

Harbs

Juergen:

One question is still open: Is there a chance to create KeyboardShortcuts for a custom menu that will work even after a ShutDown & Restart of InDesign?

They should just work. Did you try them?

Please note that you cannot create KBSCs in the scripting interface, you must do so by hand in the UI. (well, I suppose you could edit InDesign's xml file that stores this information, but that wouldn't be very...clean.)