Fix for issue #1483. Document file and handler function for each command.

[lkcampbell]

Fix for issue #1483. Document file and handler function for each command.

[redmunds]

Please update this comment to match the more helpful one below:

// File shell callbacks - string must MATCH string in native code (appshell/command_callbacks.h)

It might make sense to group all of these native callback command is together.

[lkcampbell]

@redmunds, looking at the appshell/command_callback.h, it appears there are more Commands that need this warning comment next to them. HELP_ABOUT, EDIT_UNDO, EDIT_REDO, EDIT_CUT, EDIT_COPY, EDIT_PASTE, and EDIT_SELECT_ALL.

If that is the case (please confirm this for me as I am a novice with brackets-shell code), I need to add the callbacks comment near these other commands, too. Then, if I group all the callbacks, we lose some of the menu-specific ordering. Normally, I wouldn't think much on this but since the bug is about the difficulty of finding specific command handlers in the current code base, mixing up the menu-sorted list of commands is somewhat at odds with the intent of the fix.

I am thinking: menu order sorting of the commands first, then, in each menu section, pick out the callbacks and put the "must MATCH" comment just before each group of callbacks like this:

    // EDIT
    // File shell callbacks - string must MATCH string in native code (appshell/command_callbacks.h)
    exports.EDIT_UNDO                   = "edit.undo";                  // EditorCommandHandlers.js     handleUndo()
    exports.EDIT_REDO                   = "edit.redo";                  // EditorCommandHandlers.js     handleRedo()
    exports.EDIT_CUT                    = "edit.cut";                   // EditorCommandHandlers.js     ignoreCommand()
    exports.EDIT_COPY                   = "edit.copy";                  // EditorCommandHandlers.js     ignoreCommand()
    exports.EDIT_PASTE                  = "edit.paste";                 // EditorCommandHandlers.js     ignoreCommand()
    exports.EDIT_SELECT_ALL             = "edit.selectAll";             // EditorCommandHandlers.js     _handleSelectAll()

    exports.EDIT_SELECT_LINE            = "edit.selectLine";            // EditorCommandHandlers.js     selectLine()
    exports.EDIT_FIND                   = "edit.find";                  // FindReplace.js               _launchFind()
    (...)

To summarize: Do I need to add callback comments to these other commands and, if so, does the structure presented above look good to you?

[redmunds]

Yes, we should also add that comment for those other ids. I agree with grouping by menus first, then place the ids that are not associated with a menu item at the bottom. That looks good to me.

[redmunds]

Done with code review. Just 1 minor comment.

[lkcampbell]

@redmunds, code review changes pushed.

[peterflynn]

This seems like it will be hard to keep in sync with the actual code -- I'm guessing it will drift out of date fairly quickly. I wonder if it's better to only list the module name? That probably changes less often than the function name, and once you have the module name it's trivial to find the command handler within it (that is, telling you the module name is the part with the most bang for the buck).

[redmunds]

@peterflynn The discussion on implementating this was done in issue #1483, so can you comment over there? I copied your comment over there.

[redmunds]

Merging.