Javascript Object window should allow sort properties and methods by name

RESOLVED FIXED in mozilla1.9alpha

Status

defect
--
minor
RESOLVED FIXED
16 years ago
12 years ago

People

(Reporter: cubic, Assigned: jason.barnabe)

Tracking

Trunk
mozilla1.9alpha
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

16 years ago
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:
That's one of several features I plan on implementing (someday) for the JSObject
viewer.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Core → Other Applications
Depends on: 272906
Assignee

Comment 2

13 years ago
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?
Assignee

Comment 3

13 years ago
Posted patch patch v1 (obsolete) — Splinter Review
This patch makes the properties sorted alphabetically, case-sensitive.
Assignee: dom-inspector → jason_barnabe
Status: NEW → ASSIGNED
Attachment #223253 - Flags: superreview?(neil)
Attachment #223253 - Flags: review?(timeless)
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 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" :-/
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 on attachment 223253 [details] [diff] [review]
patch v1

sr=me assuming we come up with an agreeable sort function.
Attachment #223253 - Flags: superreview?(neil) → superreview+
Assignee

Comment 8

13 years ago
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;
Assignee

Updated

13 years ago
Depends on: 339396
Assignee

Updated

13 years ago
Attachment #223253 - Flags: superreview+
Attachment #223253 - Flags: review?(timeless)
Assignee

Comment 9

13 years ago
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.
Assignee

Comment 10

13 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Bug 339396 doesn't seem to be causing errors with this patch any more.
Attachment #223253 - Attachment is obsolete: true
Attachment #238584 - Flags: superreview?(neil)
Attachment #238584 - Flags: review?(timeless)
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.
Assignee

Comment 12

13 years ago
(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 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.
Attachment #238584 - Flags: superreview?(neil) → superreview-
Assignee

Comment 14

13 years ago
Posted patch patch v3 (obsolete) — Splinter Review
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
Attachment #238584 - Attachment is obsolete: true
Attachment #238736 - Flags: superreview?(neil)
Attachment #238736 - Flags: review?(timeless)
Attachment #238584 - Flags: review?(timeless)

Updated

13 years ago
Attachment #238736 - Flags: review?(timeless) → review+
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.
Attachment #238736 - Flags: superreview?(neil) → superreview+
Assignee

Comment 16

13 years ago
Posted patch patch v4Splinter Review
Thanks, the switches to sortNumeric brought it down to 7 lines of code instead of 12.
Attachment #238736 - Attachment is obsolete: true
Assignee

Updated

13 years ago
Whiteboard: [checkin needed]
mozilla/extensions/inspector/resources/content/viewers/jsObject/jsObjectViewer.js 	1.21
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.