338383 - Implement Copy on properties in CSS Style Rules view

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[Jason Barnabe (np)]

Just like we can now copy properties in the Computed Style view, we should be able to copy properties in the CSS Style Rules view.

Comment 1

[Jason Barnabe (np)]

Attachment: patch v1

This patch moves a bunch of code that Copy in Computed Styles was using into a common place so it can be shared by both views.

Comment 2

[Alex Vincent [:WeirdAl]]

Looks pretty good to me.  One personal nit:  I realize the local code style uses anonymous functions, but I'd like to see that start to change for new code.  If timeless as reviewer agrees, I'd add a XXX comment to the files so that people changing the files in the future name their functions.

By anonymous functions, I mean:

foo: function(bar) {
}

Comment 3

[[e-mail]]

Sharing code is always good - can we go as far as writing a generic base tree view copy method implemented by calling a per-view getRowText method?

Comment 4

[[e-mail]]

... although if you can't get that to work, sr=me on the original patch.

Comment 5

[Jason Barnabe (np)]

Attachment: patch v2

This patch implements a base copy function. It expects the viewer it's run on to have a getter for selectedRowObjects, and for that function to return an array of objects that define a delimiter, a flavor, and a toString() method. I've implemented copy in Style Rules, made minor changes to Computed Style, and more major changes to DOM Node to make it use the common function. I also made some improvements to DOM Node that were necessary to make this work: it now supports copying multiple attributes, delimited by spaces, and it now supports deleting multiple attributes at once. To do this, I had to make it copy a DOM Node wrapped in a JavaScript object rather than the DOM Node itself.

Comment 6

[[e-mail]]

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

This isn't quite what I thought I envisaged, which is:
edit menu calls
  cmdEditCopy calls
    viewer.getSelectedRowObjects calls
      [inBaseTreeView].getSelectedRowObjects calls
        this.getRowObjectFromIndex (virtual) for each index

If you think this is unreasonable then I'm happy to go back to your original patch. In the meantime, some nits I noticed while skimming this patch.

>+var cmdEditCopy = {
You can't use a global. Each transaction needs to cache its selection. Otherwise undo/redo won't work.

>+                                          objectsToCopy[0].flavor + "s",
Any reason why the flavour doesn't include the "s"?

>+  getSelectedIndices: function() {
Isn't this a duplication of the view implementation?

>+        return this.getSelectedIndices().length > 0;
>+      case "cmdEditEdit":
>+        return this.getSelectedIndices().length == 1;
Please use view.selection.count here.

Comment 7

[Jason Barnabe (np)]

(In reply to comment #6)
> >+var cmdEditCopy = {
> You can't use a global. Each transaction needs to cache its selection.
> Otherwise undo/redo won't work.

Copy isn't undoable or redoable. Or it shouldn't be, at least. Bug 242729 comment 6

> >+                                          objectsToCopy[0].flavor + "s",
> Any reason why the flavour doesn't include the "s"?

Because an individual object is only one, while the array is many. (Maybe I should just pass the object with an s-less flavor instead of an array when there's only one?)

> >+  getSelectedIndices: function() {
> Isn't this a duplication of the view implementation?

Yes, but DOMNodeViewer doesn't inherit from inBaseTreeView. I tried to make it use inBaseTreeView's function, but the view references are different.

Comment 8

[Jason Barnabe (np)]

Attachment: patch v3

This implements the envisaged code described in comment 6. The inheriting views are responsible for defining getRowObjectFromIndex, the inherited view defines getSelectedIndices and getSelectedRowObjects. Since DOM Node doesn't inherit from the base view, I couldn't get that to work, so I've left it as is (except for moving a script element to ensure its using its own cmdEditCopy).

Comment 9

[[e-mail]]

(In reply to comment #7)
Sorry for all the silly questions. Thanks for the sensible answers.

Comment 10

[[e-mail]]

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

Excellent stuff, but still a few nits:

>+function cmdEditCopy(view) {
I think it would be more sensible to pass the object to be copied.

> };
>+
>+/**
>+ * Returns an array of selected indices in the tree.
>+ * @return an array of indices
>+ */
>+inBaseTreeView.prototype.getSelectedIndices =
>+function()
>+{
>+  var indices = [];
>+  var rangeCount = this.selection.getRangeCount();
>+  for (var i = 0; i < rangeCount; i++) {
>+    var start = {};
>+    var end = {};
>+    this.selection.getRangeAt(i,start,end);
>+    for (var c = start.value; c <= end.value; c++) {
>+      indices.push(c);
>+    }
>+  }
>+  return indices;
>+}
>+
>+/**
>+ * Returns an array of row objects selected in the tree.
>+ * @return an array of row objects
>+ */
>+inBaseTreeView.prototype.getSelectedRowObjects =
>+function()
>+{
>+  var declarations = [];
>+  var indices = this.getSelectedIndices();
>+  for (var i = 0; i < indices.length; i++) {
>+    declarations.push(this.getRowObjectFromIndex(indices[i]));
>+  }
>+  return declarations;
>+}
I'm not convinced we need two methods for this. Also, it should be possible to hoist these particular declarations into the prototype definition above.

>+  return new CSSDeclaration(this.getCellText(aIndex, {id: "olcStyleName"}),
>+                            this.getCellText(aIndex, {id: "olcStyleValue"}));
Rather than the two calls to getCellText with the "fake" column objects I think I'd prefer if you inlined the appropriate parts of getCellText instead. (It looks as if the same idea applies to the other versions too).

Comment 11

[Jason Barnabe (np)]

(In reply to comment #10)
> (From update of attachment 223252 [details] [diff] [review] [edit])
> I'm not convinced we need two methods for this. Also, it should be possible to
> hoist these particular declarations into the prototype definition above.

I made it separate so that future patches can use getSelectedIndices. For example, delete in Style Rules only deletes the first selected one - in the future I will modify it to loop through the selectedIndices to delete all selected. It's a pretty handy function; I believe something similar should part of the XUL tree definition.

As for not putting them into the prototype definition, comment 2 says no more anonymous functions.

Comment 12

[Jason Barnabe (np)]

Attachment: patch v4

cmdEditCopy now accepts the array of objects to copy, rather than the view that contains them.

Comment 13

[[e-mail]]

(In reply to comment #11)
>As for not putting them into the prototype definition, comment 2 says no more
>anonymous functions.
That's irrelevant. Whether you write
Foo.prototype.foo = function(bar) {};
or
Foo.prototype = { foo: function(bar) {} };
the function is still anonymous.
You need to write function foo(bar) instead.

Comment 14

[[e-mail]]

Comment on attachment 223400 [details] [diff] [review]
patch v4

>@@ -87,8 +88,43 @@ inBaseTreeView.prototype = 
>       var svc = Components.classes["@mozilla.org/atom-service;1"].getService(i);
>       return svc.getAtom(aVal);
>     } catch(ex) {
>       return null;
>     }
>   }
>   
> };
>+
>+/**
>+ * Returns an array of selected indices in the tree.
>+ * @return an array of indices
>+ */
>+inBaseTreeView.prototype.getSelectedIndices =
If you prefer, you could move the existing method out of the .prototype = { } block into a separate declaration like this one, so as to be consistent.

>+StylePropsView.prototype.getCellValue = 
>+function(aRow, aCol) 
>+{
>+  var prop = this.mDec.item(aRow);
>+  if (aCol.id == "olcPropPriority") {
>+    return this.mDec.getPropertyPriority(prop)
>+  }
>+  
>+  return null;
>+}
Are you still using this?

Comment 15

[Jason Barnabe (np)]

(In reply to comment #13)
> You need to write function foo(bar) instead.

Then I'm unclear on what comment 2's asking for. Am I supposed to write it like
function foo(bar) {}
baz.prototype.foo = foo
?

Or is just asking to not use the form foo: function(bar) {}, but other anonymous functions are fine?

Comment 16

[Alex Vincent [:WeirdAl]]

foo: function foo(bar) {
}

Comment 17

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

Or Foo.prototype.bar = function bar() {}.

Comment 18

[Jason Barnabe (np)]

Attachment: patch v5

Sorry for being a dumbass, I've never seen that syntax before.

Comment 19

[Jason Barnabe (np)]

Comment on attachment 223525 [details] [diff] [review]
patch v5

Neil already sr+'d the previous patch.

Comment 20

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

extensions/inspector/resources/content/viewers/styleRules/styleRules.js 1.26
extensions/inspector/resources/content/viewers/domNode/domNode.xul 1.13
extensions/inspector/resources/content/viewers/computedStyle/computedStyle.js 1.13
extensions/inspector/resources/content/jsutil/xul/inBaseTreeView.js 1.10
extensions/inspector/resources/content/utils.js 1.14
extensions/inspector/resources/content/viewers/styleRules/styleRules.xul 1.22