Closed Bug 179621 Opened 17 years ago Closed 9 years ago

Inspector should use Editor's TransactionManager model

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: WeirdAl, Assigned: crussell)

References

(Blocks 1 open bug, )

Details

(Keywords: helpwanted)

Attachments

(7 files, 6 obsolete files)

10.82 KB, application/vnd.mozilla.xul+xml
Details
1.85 KB, patch
timeless
: review+
Details | Diff | Splinter Review
5.67 KB, patch
timeless
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
7.29 KB, patch
timeless
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
80.34 KB, patch
WeirdAl
: review+
Details | Diff | Splinter Review
19.96 KB, patch
WeirdAl
: review+
Details | Diff | Splinter Review
12.94 KB, patch
crussell
: review+
Details | Diff | Splinter Review
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.
Keywords: helpwanted
Summary: Reconcile Inspector's undo/redo commands with Editor's undo/redo → Inspector should use Editor's TransactionManager model
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?
Status: NEW → ASSIGNED
Keywords: helpwanted
Target Milestone: --- → mozilla1.5beta
Really taking.
Assignee: hewitt → ajvincent
Status: ASSIGNED → NEW
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.
Attached file Phase One patch demo
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.
Attachment #119620 - Flags: superreview?(kin)
Attachment #119620 - Flags: review?(bugmail)
Severity: enhancement → normal
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.
Attachment #119620 - Flags: review?(bugmail) → review?(timeless)
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.
Blocks: 112922
No longer blocks: 112922
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.
Attachment #119620 - Flags: review?(timeless) → review?(caillon)
(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?
Attachment #119620 - Flags: superreview?(kinmoz)
Attachment #119620 - Flags: review?(caillon)
Attachment #119620 - Flags: review?(akkzilla)
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?
Attachment #119620 - Flags: review?(akkzilla) → review+
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...
Target Milestone: mozilla1.5beta → mozilla1.9alpha
Regarding comment 14:  I created a short testcase to see what would happen.  It
did not work.
Attachment #119620 - Attachment is obsolete: true
Comment on attachment 150463 [details] [diff] [review]
Phase One patch after r+ nits fixed

transferring r+ over and requesting sr.
Attachment #150463 - Flags: superreview?(kinmoz)
Attachment #150463 - Flags: review+
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)
Attachment #150463 - Flags: review+ → review?(daniel.glazman)
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.
Attachment #150463 - Flags: superreview?(kinmoz) → superreview?(dveditz)
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
Attachment #150463 - Flags: superreview?(dveditz)
Attachment #150463 - Flags: superreview+
Attachment #150463 - Flags: review?(daniel.glazman)
Attachment #150463 - Flags: review+
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.)
Attachment #166828 - Flags: review?(caillon)
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
Attachment #150463 - Attachment is obsolete: true
Product: Core → Other Applications
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.
Attachment #166828 - Attachment is obsolete: true
This patch (and future Phase Two patches) depends on attachment 167042 [details] [diff] [review].
Attachment #166828 - Flags: review?(caillon)
Attachment #167042 - Flags: review?(timeless)
Forgot the domNode.xul change.
Attachment #167045 - Attachment is obsolete: true
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...
Attachment #167042 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #167042 - Flags: review?(timeless)
Attachment #167042 - Flags: review+
Attachment #167042 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #167043 - Flags: review?(caillon)
Attachment #167046 - Flags: review?(caillon)
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
Attachment #167042 - Attachment description: Phase One amendment patch → Phase One amendment patch (checked in)
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.
Attachment #167043 - Flags: superreview?(bzbarsky)
Attachment #167043 - Flags: review?(caillon)
Attachment #167043 - Flags: review+
Comment on attachment 167043 [details] [diff] [review]
Phase Two patch for current DOM Nodes viewer code (checked in)

sr=bzbarsky
Attachment #167043 - Flags: superreview?(bzbarsky) → superreview+
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
Attachment #167043 - Attachment description: Phase Two patch for current DOM Nodes viewer code → Phase Two patch for current DOM Nodes viewer code (checked in)
Attachment #167046 - Flags: superreview?(simon)
Attachment #167046 - Flags: review?(caillon)
Attachment #167046 - Flags: review+
Comment on attachment 167046 [details] [diff] [review]
Phase Two patch for current DOM Node viewer code (domNode.js, domNode.xul)

Typo in sr? flag.
Attachment #167046 - Flags: superreview?(simon) → superreview?(bzbarsky)
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.
Attachment #167046 - Flags: superreview?(bzbarsky) → superreview+
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.
Assignee: ajvincent → dom-inspector
Target Milestone: mozilla1.9alpha → ---
Alex, would you be willing to pick this back up, or alternatively, would someone else be wiling to?
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
Keywords: helpwanted
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #445241 - Flags: review?(sdwilsh)
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.
Attachment #445241 - Flags: review?(sdwilsh) → review?(ajvincent)
Attachment #445245 - Flags: review?(sdwilsh) → review?(ajvincent)
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+.
Attachment #445241 - Flags: review?(ajvincent) → review-
(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.
Attachment #445245 - Flags: review?(ajvincent) → review-
(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.
OS: Windows 98 → All
Hardware: x86 → All
Attachment #445245 - Attachment is obsolete: true
Attachment #445633 - Flags: review?(ajvincent)
Attachment #445241 - Attachment description: Phase Three cleanup (domNode.xul, domNode.js, utils.js, inspector.xml, and editingOverlay.xul) → cleanup (domNode.xul, domNode.js, utils.js, inspector.xml, and editingOverlay.xul)
Attached patch phase three mkIISplinter Review
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.
Attachment #445635 - Flags: review?(ajvincent)
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.
Attachment #445633 - Flags: review?(ajvincent) → review+
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.  :)
Attachment #445635 - Flags: review?(ajvincent) → review+
Attachment #445241 - Flags: review- → review+
Attachment #445633 - Flags: superreview?(neil)
Attachment #445635 - Flags: superreview?
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.
Attachment #445635 - Flags: superreview? → superreview?(neil)
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.
Attachment #445633 - Attachment is obsolete: true
Attachment #446936 - Flags: superreview?(neil)
Attachment #446936 - Flags: review+
Attachment #445633 - Flags: superreview?(neil)
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.
Attachment #446936 - Flags: superreview?(neil) → superreview+
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.
Attachment #445635 - Flags: superreview?(neil) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.