193726 - Separate command execution from command stack insertion

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Alex Vincent [:WeirdAl]]

Currently, DOM Inspector's implementation of its command stack (aka transaction
manager, no relation to Editor's TxMgr) primarily operates through the
panelset's execCommand() method.  As currently designed, a command object has
precisely one opportunity to enter the command stack:  when the command executes
its doCommand() method for the first time.

When DOM Inspector needs to open a dialog box, it should do so with a command
which only enters the command stack after the user activates the "OK" button. 
Dialog boxes are preferable in creating nodes (bug 112775), and in inserting an
attribute or setting a style rule for an element (bug 193724).

The patch coming up is trivial; it splits execCommand() into two separate
methods.  The
bulk of execCommand() remains unchanged, with the code inside:

            if (!noPush) {
// ...
            }

being moved to a new method, addCommandToStack().  execCommand() then calls 
addCommandToStack(command), which generates precisely the same results.

For a command which invokes a dialog box, it should be defined with the following
structure:

function cmdFoo() {
  // other properties
  this.isInStack = false;
}
cmdFoo.prototype = {
  doCommand: function() {
    if (!this.conditionSatisfied) {
      window.openDialog('chrome://inspector/content/path/file.xul', 'file',
'chrome, modal=1', this);
      return true;
    }

    // execute real commands

    if (!this.isInStack) {
      viewer.pane.panelset.addCommandToStack(this);
      this.isInStack = true;
    }
    return false;
  },

  undoCommand: function() {
    // undo commands
  }
};

The dialog box should set some condition on the command object when the OK
button is activated.  
Thus the sequence of actions is this:

* User calls command to open dialog box.
* Command object initializes, with a property isInStack = false.
* execCommand() calls command.doCommand()
* doCommand() observes the required condition has not been satisfied, so it
opens a dialog box and 
  returns true to execCommand()
* execCommand() receives true for its noPush variable, and does not execute
addStackToCommand().
* If the user cancels the dialog box, nothing is lost.
* If the user activates the OK button, several steps occur:
  * the dialog box modifies the command object which it received as
window.arguments[0], including
    a flag to indicate the condition required has been satisfied.
  * the dialog box executes the command object's doCommand() method again.
  * doCommand() observes the required condition has been satisfied, so it
executes the real commands.
  * doCommand() observes the isInStack property is still false, so it executes
the panelset's
    addCommandToStack() method to officially add itself to the stack.  Then it
sets isInStack to true.
  * doCommand() then returns false, to prevent a strict warning.
* If the user then undoes the command, the isInStack property must not be modified.
* If the user then redoes the command, several steps occur:
  * doCommand() observes the required condition has been satisfied, so it
re-executes the real commands.
  * doCommand() observes the isInStack property is true.
  * doCommand() then returns false, to prevent a strict warning.

Thus the command stack may work safely with dialog boxes.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: patch to move command stack insertion into new method

Comment 2

[Alex Vincent [:WeirdAl]]

Comment on attachment 114685 [details] [diff] [review]
patch to move command stack insertion into new method

Although caillon has told me a couple times that Inspector didn't need sr=,
he's not around right now.  Better to be safe than sorry and get sr= from one
of the "when-in-doubt" people.

Comment 3

[Alex Vincent [:WeirdAl]]

Apologies to caillon:  he said it's Venkman that waives sr=, not Inspector.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 114685 [details] [diff] [review]
patch to move command stack insertion into new method

looks very reasonable.

Comment 5

[Alex Vincent [:WeirdAl]]

fixed per checkin by timeless.  Thanks, roc and timeless (and caillon)