In order to make bug 109682 work (tying DOM Inspector into the editor), we must have the undo and redo features of DOM Inspector use something nearly identical to Editor's undo/redo. Unfortunately, it looks a bit complex. According to akkana and my own research, Editor uses a C++ based commandDispatcher (chrome://editor/content/ComposerCommands.js I think). Inspector, on the other hand, uses a JS-based command stack mechanism (chrome://inspector/content/inspector.xml ). To make matters worse, I have a partial fix for bug 112922 (editing #text nodes in DOM Inspector) which unfortunately breaks undo/redo in DOM Inspector. (Hence "partial".) To fix it, I must devise a different undo/redo scheme specifically for the text nodes, which will involve multiple <xul:textbox /> elements, and tie it into DOM Inspector's undo/redo mechanisms. For bug 112922, I could modify inspector.xml to include a check for the command having a "local undo" which (if active) would force panelset.undo() to not adjust panelset.mCommandPtr as it would normally do. But then I consider the Editor as well, and I dislike that solution. I'm not saying Editor's undo/redo doesn't work, nor am I saying Inspector's undo/redo doesn't work. I am saying the two cannot, as currently written, work together. I am not familiar with Editor's undo/redo features (hence all the cc's on initial filing). I have my thoughts on how I would fix them, based on an absolute minimum of change to both tools (basically me writing new code to glue the two and bug 112922's text node edits together, so that Editor's commands, Inspector's commands, and my own text-node-edit commands would all run as "child" commands of a stack of "pseudo-commands" which control what Undo and Redo affects -- if you understand that). But I figure such would probably be fairly inefficient because Editor's undo/redo structure is so different, and Inspector doesn't lend itself to making its undo/redo commands into a child module of other undo/redo controllers (if you understand that, that's what I meant to do for editing text nodes). I don't know if Editor's undo/redo does, but it would be nice if it did. I hope some of the technical dialogue above helps explain what I mean. If not, I can provide an XUL/JS document demonstrating the sort of modularized undo/redo functionality I think we need. No, I have not started to look at copy, delete, or paste commands either. Undo and redo are more fundamental than these. Feedback?

You don't need the editor's command dispatching code to implement your own undo/redo. All you need is the TransactionManager and to modify the inspector code which makes modifications to the DOM tree, so that it uses transactions to perform the actual manipulation of the data. You can do all of this in JS, and as an example of how to do this, you can take a look at the patch in bug 160019 which adds Undo/Redo to the bookmarks code. If you use the transaction manager, you can write simple transaction primitives like "remove" and "insert", and re-use them to create more complex edits ... for example to perform a "replace" you could simply execute the "remove" and "insert" transactions in batch mode (so that they both undo at one time) or implement a "replace" transaction which simply creates and performs the "remove" and "insert" in it's do method and let the TransactionManager handle the aggregation automatically for you.

:) I figured mozilla.org had something like this. Thanks. caillon, hewitt: what do you think? Obviously, using TransactionManager is a major architectural change, and per our discussion the other day, that's not something we want to rush. Plus, to date (I think) DOM Inspector has used XUL, JS and XBL exclusively: there's no calls to XPCOM whatsoever in the Inspector code, as this will require. Personally, I'd like to implement the temporary patch for the "local undo" for bug 112922 long before we try to do this. The temporary patch is far simpler -- about four lines of JS code. On the plus side, I have a great deal of notes I was working on over the last two days, notes which become obsolete if we do decide to use TransactionManager -- but also notes which will help me formalize the fix for bug 112922.

Re comment 2: Inspector does use XPCOM/XPConnect for the flasher effect. Interesting that nsIEditor has beginTransaction() and endTransaction() methods... and uses them so profusely throughout the editor suite. It'd be really nice if the TxMgr had those methods instead, or could otherwise create do-nothing transactions we could add JS methods to. kin sent me a sample JS script for transactions and TxMgr, which wasn't guaranteed to work. I've tinkered with it six different ways from Sunday, and I still get a bunch of XPConnect method-not-implemented errors, which I've traced down to two distinct parts of the sample: where txmgr.doTransaction(transaction) happens and when I try to undo/redo. Interestingly, if I keep the number of transactions in the txmgr down to one, I don't get any of these errors.

Alex, you mentioned above that the editor has beginTransaction() and endTransaction() ... the transaction manager has beginBatch() and endBatch() which allows you to batch several transactions together into one undoable/redoable set. The only difference between the Editors begin/endTransaction() and the transaction managers begin/endBatch() is that the editor also turns off reflow and painting while it is batching the transactions together ... otherwise, they are the same thing ... in fact begin/endTransaction() calls txmgr->begin/endBatch(). I'm not exactly sure why you are getting errors when your transactions are executed. You have to make sure you use "this.myField" or "this.myTxnFunc()" from within your transaction methods when you want to access your transaction members/fields or functions. My example in bug 192569 works just fine.

kin: I don't know what was different between the sample you sent me earlier and the one you give in bug 192569; all I know is that it works be-yoo-ti-full-lee. Now that I do have a working JS-based nsITransaction object, I'm able to start tinkering around with Inspector's undo/redo scheme. This will involve a serious redesign of the panelset binding at the very least; it will almost certainly involve changes to every single cmdFoo() function in the Inspector tool, though I'm not sure how extensive those changes will be. I need to hit the drawing board to do this one right. Incidentally, kin, my thinking right now is the panelset binding should add a property named "txMgr" for the transaction manager. For bug 109682, how might that txMgr property find and reference editor.txmgr?

Really taking.

http://www.mozillazine.org/weblogs/weirdal/archives/txMgr_transition.txt for documentation and logic breakdown. I anticipate this being the first of four patches. Demo testcase coming up.

Sorry, I meant the patch would solve the first of three or four phases for this bug, not necessarily be the first patch of four...

Comment on attachment 119620 [details] [diff] [review] Phase One patch: Transitional changes to Inspector's stack model I'm thinking kin should sr= this one, as it deals quite heavily with TxMgr and some theory of transaction managers. sicking: I know you're a bit busy with r= requests; if you don't want to r= this one, can you nominate someone else? (I'm not expecting caillon to be around.) For attachment 119621 [details], you may need to run it from chrome to see it working.

Comment on attachment 119620 [details] [diff] [review] Phase One patch: Transitional changes to Inspector's stack model my review queue is very long atm, and I won't be doing any reviewing the comming week due to vacation. Of course I can still do this one, but not anytime soon.

The theory I came up with for the transitional TxMgr scheme breaks in the case of a zero-transaction TxMgr. That is very possible from a standpoint of an xul:textbox element (with TxMgr for the input field transferred out to an external routine, as kin and I have been discussing), assuming the user makes zero changes to the text node. I've rearranged the logic about four different times. No resolution in sight. Broken logic means broken implementation. It'd be best if XUL textboxes had a single TxMgr to deal with in this case.

Comment on attachment 119620 [details] [diff] [review] Phase One patch: Transitional changes to Inspector's stack model >+ if ((!enabled) && (this.mCommandPtr > -1)) { i wouldn't parenthesize !enabled >+ <method name="createTransactionManager"> >+ <body><![CDATA[ >+ var txmgr = Components.classes["@mozilla.org/transactionmanager;1"].createInstance(); >+ txmgr = txmgr.QueryInterface(nsITransactionManager); please do: var txmgr = Components.classes["@mozilla.org/transactionmanager;1"].createInstance(nsITrans actionManager); >+ // command.txnType != "undefined"; we have an nsITransaction. this logic /seems/ bad, why not do foo instanceof nsITransaction? >+ if (command.txnType == "dialog") { >+ // transaction executed from dialog box onAcceptDialog event; open dialog. >+ command.openDialog(); >+ return; > } this behavior seems strange. can you explain why this is the thing to do? >+ if ((command instanceof nsITransactionManager)&&(command.numberOfUndoItems > 0)) { missing whitespace around &&. sorry, i can't review beyond this... i should have done this little ages ago.

(In reply to comment #12) Oogh, reviewing a patch over a year old? I hope it hasn't bitrotted! > >+ // command.txnType != "undefined"; we have an nsITransaction. > > this logic /seems/ bad, why not do foo instanceof nsITransaction? Because we aren't using a "var x = new nsITransaction()". There's no nsITransaction constructor that I know of. > > >+ if (command.txnType == "dialog") { > >+ // transaction executed from dialog box onAcceptDialog event; open dialog. > >+ command.openDialog(); > >+ return; > > } > > this behavior seems strange. can you explain why this is the thing to do? The idea is that opening a dialog is not in and of itself an undoable command. The actual command executes after the dialog has set all the parameters. (If the user presses cancel, then we don't want anything to have taken place as far as the command stack goes.)

foo instanceof Components.interfaces.nsITransaction

Comment on attachment 119620 [details] [diff] [review] Phase One patch: Transitional changes to Inspector's stack model I really don't understand these editor APIs enough to offer a passable review, but it looks ok. Akkana, what do you think?

Comment on attachment 119620 [details] [diff] [review] Phase One patch: Transitional changes to Inspector's stack model I'm not really up on this API either, but the changes seem reasonable. Near the end: constructor.prototype.QueryInterface = ConvertCommandToTxn.txnQueryInterface Is the missing semicolon intentional?

I doubt it is, not that it matters with the way JavaScript parsing works -- the semi-colon is implicit in that context. Still, it should be added for consistency. Al, can you address that and the previous comments?

Certainly. Good catch on that semicolon... and I'll also have to recheck the patch for bitrotting. If there are any bitrots, I'll let you know and run it by caillon for re-review. Please note once again http://www.mozillazine.org/weblogs/weirdal/archives/txMgr_transition.txt at the very end for what "Phase Two" and "Phase Three" will be. I'm trying to keep these patches manageable for reviewers...

Regarding comment 14: I created a short testcase to see what would happen. It did not work.

Comment on attachment 150463 [details] [diff] [review] Phase One patch after r+ nits fixed transferring r+ over and requesting sr.

sfraser, glazou: I haven't heard anything from kin regarding potential super-review of my patch. As the reviewers were not solid on the API, I'd appreciate having an opinion on that. bz suggested the two of you. glazou: if you do know the API, an extra review from you would suffice for me to request a general sr.

Comment on attachment 150463 [details] [diff] [review] Phase One patch after r+ nits fixed per discussion with glazou, r? him (Just looking to check the txmgr api usage)

Comment on attachment 150463 [details] [diff] [review] Phase One patch after r+ nits fixed requesting sr from a catch-all. This patch has gotten r+ from caillon and akkana. Comment 21 is still relevant. This patch has also sat, basically unchanged, for 18 months. It is seriously blocking a lot of the work I want to do on Inspector. Please, somebody give me a sr+ or sr- and tell me what to fix, so we can start getting traction on Inspector bugs.

Comment on attachment 150463 [details] [diff] [review] Phase One patch after r+ nits fixed There is a point at which it's better to just check stuff in and catch bugs later than to spin endlessly waiting for reviews. You passed that point a long time ago. Rubber-stamp r+sr=roc

Merci beaucoup. This work isn't important enough to ask for approval-foo on, so hopefully we'll land the first patch after the freeze lifts. In the meantime, I'll run another check for bitrotting and start posting patches for each panel.

A couple nits: I realized that the ConvertCommandToTxn function in my Phase One patch is really unnecessary and undesirable, considering that function will eventually be removed from the source code in a later edition. So it's best just to get it out of the way now. The changes you see in dom.js are extremely typical of the changes I intend to make across DOM Inspector's viewers for this bug.

Comment on attachment 166828 [details] [diff] [review] Phase Two Patch (utils.js, dom.js) Note this patch is dependent on attachment 150463 [details] [diff] [review] landing. (Also note 150463 is currently contaminated with <cr> characters... I wrote that patch long before I started building on Linux.)

Comment on attachment 150463 [details] [diff] [review] Phase One patch after r+ nits fixed mozilla/extensions/inspector/resources/content/inspector.xml 1.17 mozilla/extensions/inspector/resources/content/utils.js 1.9

I'm splitting up attachment 166828 [details] [diff] [review] (the original Phase Two patch), so that this one (which is more important and blocks other work) can get reviews faster.

This patch (and future Phase Two patches) depends on attachment 167042 [details] [diff] [review].

Forgot the domNode.xul change.

What a surprise: Apparently, DOM Inspector has commands only for the DOM Nodes and DOM Node viewers. The following viewers do not have command objects at all: * boxModel * computedStyle * jsObject * styleRules * stylesheets * xblBindings So there's only two patches required for Phase Two on current code. Other DOM Inspector bugs might also have Phase Two patches (for adding new functionality to Inspector), but other than that we're covered...

Comment on attachment 167042 [details] [diff] [review] Phase One amendment patch (checked in) phase one amendment checked in Checking in extensions/inspector/resources/content/utils.js; /cvsroot/mozilla/extensions/inspector/resources/content/utils.js,v <-- utils.js new revision: 1.10; previous revision: 1.9 done

Comment on attachment 167043 [details] [diff] [review] Phase Two patch for current DOM Nodes viewer code (checked in) >+++ extensions/inspector/resources/content/viewers/dom/dom.xul 2004-11-25 09:48:39.000000000 -0800 >- <script type="application/x-javascript" src="chrome://inspector/content/viewers/dom/dom.js"/> > <script type="application/x-javascript" src="chrome://inspector/content/utils.js"/> ... >+ <script type="application/x-javascript" src="chrome://inspector/content/viewers/dom/dom.js"/> please have the person who commits this patch commit this change first with a message explaining that it's to allow dom.js access to txnQueryInterface.

Comment on attachment 167043 [details] [diff] [review] Phase Two patch for current DOM Nodes viewer code (checked in) sr=bzbarsky

Comment on attachment 167043 [details] [diff] [review] Phase Two patch for current DOM Nodes viewer code (checked in) Checking in mozilla/extensions/inspector/resources/content/viewers/dom/dom.js; /cvsroot/mozilla/extensions/inspector/resources/content/viewers/dom/dom.js,v <-- dom.js new revision: 1.34; previous revision: 1.33 done Checking in mozilla/extensions/inspector/resources/content/viewers/dom/dom.xul; /cvsroot/mozilla/extensions/inspector/resources/content/viewers/dom/dom.xul,v <-- dom.xul new revision: 1.16; previous revision: 1.15 done

Comment on attachment 167046 [details] [diff] [review] Phase Two patch for current DOM Node viewer code (domNode.js, domNode.xul) Typo in sr? flag.

Comment on attachment 167046 [details] [diff] [review] Phase Two patch for current DOM Node viewer code (domNode.js, domNode.xul) It'll take me a few days to get to this, at best.

Comment on attachment 167046 [details] [diff] [review] Phase Two patch for current DOM Node viewer code (domNode.js, domNode.xul) >+ merge: txnMerge, Could we make txnMerge have an explicit nsITransaction arg where it's declared? Just so it's clear what's going on? I wonder whether some sort of shared proto would be a good idea here too given all the code duplication. But sr=bzbarsky as-is, with the txnMerge nit. Let me know if you need this checked in.

Reassigning DOM-I bugs which have stagnated in my buglist back to default owner. Hopefully someone will pick up some of these bugs and work on them. I'll continue to follow them.

Alex, would you be willing to pick this back up, or alternatively, would someone else be wiling to?

Comment on attachment 445241 [details] [diff] [review] cleanup (domNode.xul, domNode.js, utils.js, inspector.xml, and editingOverlay.xul) Let's have Alex look at these first.

Comment on attachment 445241 [details] [diff] [review] cleanup (domNode.xul, domNode.js, utils.js, inspector.xml, and editingOverlay.xul) All this patch does is reformat the code - whitespace changes, inserting brackets, etc. - and removing one probably unused function (dumpDOM). In other words, you're shredding hg blame without adding or changing how the code works. This is generally frowned upon. Don't get me wrong - cleanup is a good idea, but cleanup for cleanup's sake only is not enough. From the Code Review FAQ: "is the issue being fixed actually a bug?" If this is a bug, it's cosmetic at best. I need a really good reason to mark this r+.

(In reply to comment #46) See patch #2 (real changes). Patch #1 is cleanup for files touched in patch #2. I don't know why Shawn marked you for review on this one, too. Shawn, did you mean for Alex to review the cleanup patch as well?

Comment on attachment 445245 [details] [diff] [review] Phase Two for last remaining inspector-style commands (domNodes.js) + Phase Three >diff -r 35bfe39609b5 -r b7917693bd8f resources/content/inspector.xml > <method name="execCommand"> > <parameter name="aCommand"/> > <body><![CDATA[ ... >+ this.mTransactionManager.doTransaction(command); I've been thinking about the transaction manager model in a more abstract form the last few years, and I've since come to realize there's a flaw in my original approach. Namely speaking: doTransaction should not be called on an incomplete transaction, especially one that can be cancelled. For example - and I think I'm the one that botched this many years ago - let's say I request we insert a node. In dom.js, this means creating a new instance of InsertNode(). Here, doTransaction() opens a new dialog. Then, the user hits cancel. The result is a null-op transaction stored in the txmgr. Oops. This causes problems because (a) the user clicks undo, then (b) the user clicks redo and sees nothing change again. Here, you're making the same mistake I did. I tried to write around this by having a txnType property of the prototype. Unfortunately, this seems unused. InsertNode should've had a special txnType of "dialog" because, unless the dialog succeeds, nothing's changed and you don't have a real transaction. Instead, execCommand should check to see if a dialog is required to execute the command. If so, open the dialog (modal!) and check for accept. If the dialog's accepted, then the transaction should be done - otherwise, deleted. (If you decide to make the dialog non-modal, then cancel the dialog once it loses window focus. Order of operations is very important for undo/redo, and a canceled dialog means it can't appear out of order later.) If you see the word "openDialog" in current code, and current code relies on the response... that's probably a good candidate. For the null-op transaction reason, I'll be marking the patch r-. Most of the patch is fine, by the way... once you fix this, it'll almost certainly get r+. > function cmdEditCut() {} > cmdEditCut.prototype = > { >+ // required for nsITransaction >+ QueryInterface: txnQueryInterface, >+ merge: txnMerge, >+ isTransient: false, >+ > cmdCopy: null, > cmdDelete: null, >- doCommand: function DNVr_Cut_DoCommand() >+ doTransaction: function DNVr_Cut_DoTransaction() > { > if (!this.cmdCopy) { > this.cmdDelete = new cmdEditDelete(); > this.cmdCopy = new cmdEditCopy(viewer.selectedAttributes); > } > this.cmdCopy.doTransaction(); >- this.cmdDelete.doCommand(); >+ this.cmdDelete.doTransaction(); > }, I'm debating this one. In a redo call, this.cmdCopy.doTransaction gets called twice. That means we overwrite the clipboard on a redo - which is not desirable. > cmdEditInsert.prototype = > { ... > promptFor: function DNVr_Insert_PromptFor() > { ... > }, > >- doCommand: function DNVr_Insert_DoCommand() >+ doTransaction: function DNVr_Insert_DoTransaction() > { > if (!this.attr) { >- return this.promptFor(); >+ this.promptFor(); > } Again, if promptFor results in a cancellation, you'll have a null-op transaction in the txmgr.

(In reply to comment #48) I agree that those commands are buggy, but those are bugs that exist in DOM Inspector even today. I'm up for filing and fixing a bug along the lines of bug 546170 but for the DOM Nodes viewer, but those things are more about command logic and not so much about making sure we're using Transaction Manager through-and-through. > Instead, execCommand should check to see if a dialog is required to execute > the command. If so, open the dialog (modal!) and check for accept. If the > dialog's accepted, then the transaction should be done - otherwise, deleted. > (If you decide to make the dialog non-modal, then cancel the dialog once it > loses window focus. Order of operations is very important for undo/redo, and > a canceled dialog means it can't appear out of order later.) If you see the > word "openDialog" in current code, and current code relies on the response... > that's probably a good candidate. Things don't even have to get that complicated. Instead, the prompting should just be moved out of the transactions themselves. We should do in domNodes.js the same thing that we do with the CSS Rules viewer, which is to do the prompting in getCommand, and have getCommand return null if we get a negative response.

I split up phase two and three for this round. Alex, please indicate if you stand by r- on the cleanup. For what it's worth, this is how I've done with all the other touched files for inspector bugs, and in my opinion, poorly or inconsistently formatted code is worse than the one extra step that it takes to figure out blame.

I'll get back to you sometime in the next two days. (That's my personal goal when it comes to reviews. Having been burned waiting 18 months for reviews once before, I don't like to mess around when someone asks me to review!)

(In reply to comment #47) > Shawn, did you mean for Alex to review the cleanup patch as well? Uh, no. Admittedly, I didn't look that closely before redirecting the reviews. I can do that. Alex - I'm fine with cleaning up the code in these files in the first step of fixing a bug. Colby has been doing that in lots of other places too. In the world of hg with merges, blame is easily polluted, and it's not so hard to go back just one more revision. It's a onetime cost that will get us more consistent code for the future.

Comment on attachment 445635 [details] [diff] [review] phase three mkII Apologies for my earlier sharpness - with sdwilsh as module owner saying cleanup's okay, I have no objection whatsoever. :)

Comment on attachment 445635 [details] [diff] [review] phase three mkII Argh. That's odd. I submitted the form with a blank sr field before the corrected one.

Comment on attachment 445633 [details] [diff] [review] phase two with extra domNodes.js fixes Both promptFor implementations actually perform the edit "outside" of the transaction - I guess this just means it does the same work twice? >- return false; >+ >+ return out.accepted ? this : null; > } > } >- return true; > }, Doesn't this need to be return null?

Well, the return statement was inside the wrong block. Things are different now.

Comment on attachment 446936 [details] [diff] [review] phase 2 mkIII; correct doPrompt return value and use command instance as domNodeDialog outparam >+ return this.accepted ? this : null; ... >+ if (this.accepted) { >+ return this; > } >+ >+ return null; Could use ? : here too.

Comment on attachment 445635 [details] [diff] [review] phase three mkII >+ var tmClassID = "@mozilla.org/transactionmanager;1"; >+ var tmIface = "nsITransactionManager"; >+ var tm = XPCU.createInstance(tmClassID, tmIface); >+ this.mTransactionManager = XPCU.QI(tm, tmIface); Nit: don't need to QI, createInstance already does that.

Pushed attachment 445241 [details] [diff] [review]: http://hg.mozilla.org/dom-inspector/rev/50c6ea957231 (In reply to comment #58) > (From update of attachment 446936 [details] [diff] [review]) > >+ return this.accepted ? this : null; > ... > >+ if (this.accepted) { > >+ return this; > > } > >+ > >+ return null; > Could use ? : here too. Pushed with that change: http://hg.mozilla.org/dom-inspector/rev/664da10d0829 (In reply to comment #59) > (From update of attachment 445635 [details] [diff] [review]) > >+ var tmClassID = "@mozilla.org/transactionmanager;1"; > >+ var tmIface = "nsITransactionManager"; > >+ var tm = XPCU.createInstance(tmClassID, tmIface); > >+ this.mTransactionManager = XPCU.QI(tm, tmIface); > Nit: don't need to QI, createInstance already does that. (In reply to comment #59) > (From update of attachment 445635 [details] [diff] [review]) > >+ var tmClassID = "@mozilla.org/transactionmanager;1"; > >+ var tmIface = "nsITransactionManager"; > >+ var tm = XPCU.createInstance(tmClassID, tmIface); > >+ this.mTransactionManager = XPCU.QI(tm, tmIface); > Nit: don't need to QI, createInstance already does that. Pushed with that change: http://hg.mozilla.org/dom-inspector/rev/e5f26e230cd3