341894 - Style rules view should allow for Undo and handle multiple/no selection better

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Jason Barnabe (np)]

-Insert, Delete, and Toggle Important should all be undoable and redoable
-Insert, Delete, and Toggle should be disabled when no rule is selected (bug 208637)
-Toggle and Delete should be disabled when no declarations are selected and should work on all declarations selected
-Edit should be disabled unless one declaration is selected
-Edit and Toggle should not change the order of the declarations
-Insert and Delete should work from the Edit menu

Comment 1

[Jason Barnabe (np)]

Attachment: patch v1

...and the rules tree should only allow for single selection because we don't do anything useful with multiple selection (yet).

Comment 2

[Alex Vincent [:WeirdAl]]

Looks ok to me, aside from a couple minor nits (whitespacing, and a line that's unnecessarily more than 80 characters).

Comment 3

[timeless]

Comment on attachment 226027 [details] [diff] [review]
patch v1

>+        return new cmdEditDelete(this.getSelectedDec(),
>+                                     this.mPropsView.getSelectedRowObjects());

general nit, sometimes you get the alignment right (and sometimes not, e.g. here)
...(this...,
    this...);
   ^^

Comment 4

[[e-mail]]

Comment on attachment 226027 [details] [diff] [review]
patch v1

>+      case "cmdEditInsert":
>+        return this.mRuleTree.view.selection.count > 0;
Does insert really need a selection?

>+  propOnpopupshowing: function propOnpopupshowing()
Nit: bad intercaps - should be propOnPopupShowing

>+  this.oldPriority = aRule.getPropertyPriority(aProperty);
>+  this.newPriority = aNewPriority;
Aren't these the same thing?

>+  redoTransaction: function redoTransaction()
>+  {
>+    this.doTransaction();
>+    viewer.mPropsBoxObject.invalidate();
>+  }
I'd rather doTransaction did all the invalidating (it only works currently because the focus changing invalidates the selection).

Comment 5

[Jason Barnabe (np)]

(In reply to comment #4)
> (From update of attachment 226027 [details] [diff] [review] [edit])
> >+      case "cmdEditInsert":
> >+        return this.mRuleTree.view.selection.count > 0;
> Does insert really need a selection?
That's a rule selection, not a declaration selection. We need a rule to put the declaration in.

> >+  this.oldPriority = aRule.getPropertyPriority(aProperty);
> >+  this.newPriority = aNewPriority;
> Aren't these the same thing?
Right now, yeah, but it's not hard to imagine in the future the edit dialog will let you specify importance.

Comment 6

[Jason Barnabe (np)]

Attachment: patch v2

Comment 7

[[e-mail]]

Comment on attachment 227928 [details] [diff] [review]
patch v2

>+      case "cmdEditInsert":
>+        return this.mRuleTree.view.selection.count > 0;
I think I'd prefer this to say == 1;

>+    viewer.mPropsBoxObject.beginUpdateBatch();
>+    this.rule.setProperty(this.property, this.value, this.priority);
>+    viewer.mPropsBoxObject.endUpdateBatch();
I know this was copied but setProperty can throw an exception, and not ending the batch makes the tree very annoyed...

>+    this.sheet.removeProperty(this.property);
Should be this.rule, no?

>+  redoTransaction: function redoTransaction()
I didn't seem to be able to get undo/redo working at all.

>+      var newPriority = 
>+        this.rule.getPropertyPriority(this.declarations[i].property) == "" ?
>+        "important" : "";
>+      this.rule.setProperty(this.declarations[i].property,
>+                            this.declarations[i].value, newPriority);
I wasn't able to make a property unimportant.

Comment 8

[Jason Barnabe (np)]

Attachment: patch v3

The problem with Undo/Redo on insert was indeed "this.sheet" instead of "this.rules". I guess forgot to change that one.

The problem with toggle importance is bug 305761. The previous code deleted the property and made a new one at the correct importance. I've switched back to that method and included an explanatory comment. Sorry for not catching that in testing.

Comment 9

[[e-mail]]

Comment on attachment 227959 [details] [diff] [review]
patch v3

>+    switch (aCommand) {
>+      case "cmdEditCopy":
>+      case "cmdEditDelete":
>+      case "cmdTogglePriority":
>+        return this.mPropsTree.view.selection.count > 0;
>+      case "cmdEditInsert":
>+        return this.mRuleTree.view.selection.count == 1;
>+      case "cmdEditEdit":
>+        return this.mPropsTree.view.selection.count == 1;
After all that, something doesn't seem to be updating correctly; both the popup and the edit menu tended to be unnecessarily disabled.

>+    try {
>+      for (var i = 0; i < this.declarations.length; i++)
>+        this.rule.removeProperty(this.declarations[i].property);
I would have thought removeProperty would be fairly safe...


>+    try {
>+      for (var i = 0; i < this.declarations.length; i++)
>+        this.rule.setProperty(this.declarations[i].property,
...as would setting a property that was previously removed.

>+                              this.declarations[i].value, 
Nit: trailing space (tried jst-review recently?)

>     <popup id="ppStyleRuleContext" onpopupshowing="return viewer.onpopupshowingRulePopup()">

>+    <popup id="ppStylePropsContext" onpopupshowing="viewer.propOnPopupShowing(this)">
Hmm, I hadn't noticed before, but I wonder whether we should try to follow existing naming convention and use onPopupShowingPropsPopup

Comment 10

[Jason Barnabe (np)]

Attachment: patch v4

Comment 11

[Jason Barnabe (np)]

(In reply to comment #9)
> After all that, something doesn't seem to be updating correctly; both the popup
> and the edit menu tended to be unnecessarily disabled.

The only situation I've found where it was incorrect is when you select a rule then click in the declarations tree without actually choosing a declaration. In that case, Insert was disabled. I've included an updateAllCommands on rule select. If there are other situations after this change where it's still wrong, let me know what they are.

> I would have thought removeProperty would be fairly safe...
> ...as would setting a property that was previously removed.

Misunderstood your previous comment. Removed.

> Nit: trailing space (tried jst-review recently?)

Had never heard of it. It's a very useful utility. I've run the updated patch through it.

> Hmm, I hadn't noticed before, but I wonder whether we should try to follow
> existing naming convention and use onPopupShowingPropsPopup

If "onpopupshowingRulePopup" follows the existing convention, I think we should change the existing convention.

Comment 12

[[e-mail]]

I just happened to notice the weird effect the leading ./ has on the Bugzilla diff viewer - mozill./extensions/inspector/resources (etc) ;-)

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/extensions/inspector/resources/content/viewers/styleRules/styleRules.xul 	1.23
mozilla/extensions/inspector/resources/content/viewers/styleRules/styleRules.js 	1.27
<and a bunch of locale files>