226819 - Javascript Object window should allow sort properties and methods by name

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Ruslan U. Zakirov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ru-RU; rv:1.4.1) Gecko/20031116
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ru-RU; rv:1.4.1) Gecko/20031116

Sometimes it very hard to find method or property by name.
Also other windows should allow 'order by' feature if it is possible.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Alex Vincent [:WeirdAl]]

That's one of several features I plan on implementing (someday) for the JSObject
viewer.

Comment 2

[Jason Barnabe (np)]

Instead of making it sort*able*, could we not just make it sort*ed*? There isn't really any useful way of sorting that view other than alphabetical by property name, is there?

Comment 3

[Jason Barnabe (np)]

Attachment: patch v1

This patch makes the properties sorted alphabetically, case-sensitive.

Comment 4

[Alex Vincent [:WeirdAl]]

I'm a little concerned about sorting it automatically - sometimes the natural order is useful (properties from nsIDOMNode, for example, are grouped together naturally).

Comment 5

[[e-mail]]

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

I quite like the idea of sorting named properties alphabetically. However, numbered properties are a different issue... "10" < "9" :-/

Comment 6

[[e-mail]]

Something like this, but better ;-)

function tryToCompareNumerically(a, b) { return a - b || a.localeCompare(b); }

This fails because "1a" sorts before "2" sorts before "10" sorts before "1a".

My next try was return parseInt(a, 10) - parseInt(b, 10) || a.localeCompare(b);

That still sorts "02" after "1", but maybe that's good enough?

Comment 7

[[e-mail]]

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

sr=me assuming we come up with an agreeable sort function.

Comment 8

[Jason Barnabe (np)]

I don't think just going by numerics is a good idea, because then you'd have for example

offsetLeft       0
offsetTop        0
ELEMENT_NODE     1
ATTRIBUTE_NODE   2
(...)
offsetWidth   1100
offsetHeight  2400
blur      function

How about putting constants first, sorted numerically then alphabetically (are constants always/usually numeric?), then non-constants sorted alphabetically? So you'd get

ELEMENT_NODE     1
ATTRIBUTE_NODE   2
(...)
blur      function
(...)
offsetHeight  2400
offsetLeft       0
offsetTop        0
offsetWidth   1100

I don't know if there's a better way to figure if something's a constant or not, but I could just go name.toUpperCase() == name;

Comment 9

[Jason Barnabe (np)]

I've got a patch ready implementing what I describe in comment 8, but it's currently failing on the left side because of bug 339396.

Comment 10

[Jason Barnabe (np)]

Attachment: patch v2

Bug 339396 doesn't seem to be causing errors with this patch any more.

Comment 11

[Alex Vincent [:WeirdAl]]

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

>+function JSPropertySort(object) {
>+  this.object = object;
>+ /**
>+  * Compares the two passed property names of the object.
>+  * @param one object to compare
>+  * @param another object to compare
>+  * @return -1 if a should come before b, 1 if b should come before a, 0 if
>+  *   they are equal
>+  */
>+  this.sort = function(a, b) {

Two nits.  (1) object is now a property of this, so why not declare the sort function as a method of JSPropertySort.prototype?

(2) Name the function, please.  function namedSortFunction(a, b).  Venkman users will thank you.

>+    // assume capitalized property names are constants
>+    var aIsConstant = a == a.toUpperCase();
>+    var bIsConstant = b == b.toUpperCase();
>+    // constants come first
>+    if (aIsConstant) {
>+      if (bIsConstant) {
>+        // both are constants, go by their value
>+        var aValue = object[a];
>+        var bValue = object[b];
>+        var aIsNumeric = parseInt(aValue, 10) == aValue;
>+        var bIsNumeric = parseInt(bValue, 10) == bValue;

Why should these checks be inside the isConstant checks?  Also, why not just use:

aIsNumeric = !isNaN(aValue);


>+        // numerics come first
>+        if (aIsNumeric) {
>+          if (bIsNumeric) {
>+            // go by their numerical value
>+            var compareResult = parseInt(aValue, 10) - parseInt(bValue, 10);

var compareResult = Number(aValue) - Number(bValue)... again, why parseInt?

>+          return -1;
>+        } else if (bIsNumeric)

Else should not follow return.

Comment 12

[Jason Barnabe (np)]

(In reply to comment #11)
> Why should these checks be inside the isConstant checks?

With this patch, constants sort by value, non-constants sort by name.

>Also, why not just use:
>
>aIsNumeric = !isNaN(aValue);

Is this document incorrect? http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Functions:isNaN
"isNaN returns true if passed NaN, and false otherwise."

>var compareResult = Number(aValue) - Number(bValue)... again, why parseInt?

Why not?

Comment 13

[[e-mail]]

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

>+/**
>+ * A sorter for the JavaScript Object view. Sort order: numeric constants,
>+ * non-numeric constants, variables. The tie-breaker for numeric constants is
>+ * the numeric value and the property name, the tie-breaker for all others is
>+ * just the property name.
>+ * @param the object whose properties will be sorted
>+ */
Why does this need to be an object? I'm not even sure an object works (i.e. sort might get called with an inappropriate this).

>+        if (aIsNumeric) {
>+          if (bIsNumeric) {
I'd go with isNaN or typeof here.

>+            // go by their numerical value
>+            var compareResult = parseInt(aValue, 10) - parseInt(bValue, 10);
>+            if (compareResult == 0) 
>+              // numeric constants with the same value, go by name
>+              return a.localeCompare(b);
>+            return compareResult;
Just use compareResult || a.localeCompare(b); or inline the subtraction.

>+    // neither are constants, go by name
>+    return a.localeCompare(b);
As per comment 5, this doesn't work on arrays... in this case, a and b are both numeric, and therefore need to be sorted numerically.

Comment 14

[Jason Barnabe (np)]

Attachment: patch v3

Addresses all comments.

I've tweaked the sort order a bit to handle arrays better: constants with numeric values sorted numerically by value then alphanumerically by name, all other constants sorted alphanumerically by name, non-constants with numeric names sorted numerically by name (ex: array indices), all other non-constants sorted alphanumerically by name. (Forgive the contrived example)

ELEMENT_TYPE = 1
APP_MODE = 2
DOCUMENT_TYPE = 2
DEFAULT_NAMESPACE = HTML
URI = http://example.com
0 = foo
1 = bar
2 = baz
10 = nih
app = Firefox
extension = domi
version = 3

Comment 15

[[e-mail]]

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

>+    function sortNumeric(a, b) {
>+      var aIsNumeric = !isNaN(a);
>+      var bIsNumeric = !isNaN(b);
I'd say it's clear what isNaN is testing, and you only use these once (although you have to lines using bIsNumeric you never execute both), so you could inline them. Having done that, you could switch the then and "else" cases, thus saving you the !s. You could even consider using ?: to save a bit more code.

>+          return Number(a) - Number(b);
Doesn't a - b; work here?

I couldn't seem to expand a document's defaultView but it turns out it's not your patch's fault after all.

Comment 16

[Jason Barnabe (np)]

Attachment: patch v4

Thanks, the switches to sortNumeric brought it down to 7 lines of code instead of 12.

Comment 17

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

mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js 	1.21