Closed Bug 967319 Opened 8 years ago Closed 6 years ago

Show a nodesList result with natural order

Categories

(DevTools :: Object Inspector, defect)

29 Branch
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: striptm, Assigned: bmax1337, Mentored)

References

Details

Attachments

(1 file, 5 obsolete files)

The results of a nodeList / HTMLCollection / ... are alphanumerical sorted. For example
document.getElementsByTagName("a");
show: 0, 1, 10, 11, 12...

It would be desirable to shew in natural order: 0, 1, 2, 3...
STR:
- Open the webconsole on this bugzilla page
- Evaluate document.getElementsByTagName("a");
- Click on the output link to open the variablesview

=> The list of <a> DOMNodes is sorted like 0,1,10,11,12,...
=> It would indeed be better to have it sorted by numerical index. Especially that we now have the highlight on hover in the variablesview.

However, if you evaluate [1,2,3,4,5,6,7,8,9,10,11,12] in the webconsole and click on the output link, the result in the variablesview is sorted as expected.
Duplicate of this bug: 1078993
Duplicate of this bug: 1130392
To fix this bug, you will need to edit http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm?rev=0f7b2674c64a#1398 and http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm?rev=0f7b2674c64a#2235 to take a custom sorting function that will order properties that parse as numbers numerically (in increasing order), and properties that do not parse as numbers lexicographically.
Assignee: nobody → bmax1337
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Attached patch 230924.diff (obsolete) — Splinter Review
Fixed bug by implementing a function naturalSort, also wrote a test. Semi-confused on why there was a test that checks to make sure the keys are "not-sorted" but yet still passes with my fix.
Attachment #8571586 - Flags: review?(jaws)
Comment on attachment 8571586 [details] [diff] [review]
230924.diff

Review of attachment 8571586 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good, just some minor style issues to address and a couple more cases to add to the test.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1396,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted && aKeysType != "just-numbers") {
> +      names.sort(
> +          this._naturalSort
> +      );

We can put this all on one line, just `names.sort(this._naturalSort);`

@@ +1606,5 @@
>    /**
> +   * Sort in ascending order.
> +   * @param string a 
> +   * @param string b 
> +   * @return boolean

This should be:
@return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0

@@ +1609,5 @@
> +   * @param string b 
> +   * @return boolean
> +   */
> +  _naturalSort: function(a,b) 
> +   { 

nit, please follow the style of how functions are defined in this file, by placing the opening curly bracket on the same line as the function signature. Please also remove the trailing whitespace on this line and the lines above it.

@@ +1611,5 @@
> +   */
> +  _naturalSort: function(a,b) 
> +   { 
> +    if (isNaN(parseFloat(a)) && isNaN(parseFloat(b)))
> +      return (a<b ? -1 : 1);

another thing that this file is doing is wrapping all if-blocks with curly brackets even if they only have one line in their body.

@@ +2247,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted) {
> +      propertyNames.sort(
> +          this._naturalSort
> +      );

Ditto about putting this all in one line.

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +121,5 @@
> +    is(keyIterator.next().value, "1", "First key should be 1");
> +    is(keyIterator.next().value, "4", "Second key should be 4");
> +
> +    // If the properties are sorted, the next one will be 10.
> +    is(keyIterator.next().value, "10", "Third key is not 10");

Please also add two more checks to make sure that the next one from here is "abc" and the one following that is "hello".
Attachment #8571586 - Flags: review?(jaws) → feedback+
Attached patch 230924.diff (obsolete) — Splinter Review
New patch matching current code conventions and better test.
Attachment #8571586 - Attachment is obsolete: true
Attachment #8572400 - Flags: feedback?(jaws)
Comment on attachment 8572400 [details] [diff] [review]
230924.diff

Review of attachment 8572400 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I should have given some of these comments in the previous review.

With the following changed this should be ready for check-in.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1604,5 @@
>    /**
> +   * Sort in ascending order.
> +   * @param string a
> +   * @param string b
> +   * @return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0

replace "0 if order no change in order" with "0 if no change in order" :)

@@ +1608,5 @@
> +   * @return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0
> +   */
> +  _naturalSort: function(a,b) {
> +    if (isNaN(parseFloat(a)) && isNaN(parseFloat(b))) {
> +      return (a<b ? -1 : 1);

Can you please add a comment saying that this only needs to handle non-numbers since it is used with an array which has numeric-based indices placed in order?

Also, put spaces around the less-than operator and remove the parentheses.
Attachment #8572400 - Flags: feedback?(jaws) → feedback+
Attached patch 230924.diff (obsolete) — Splinter Review
Attachment #8572400 - Attachment is obsolete: true
Attachment #8572427 - Flags: feedback?(jaws)
Attached patch 230924.diff (obsolete) — Splinter Review
Attachment #8572427 - Attachment is obsolete: true
Attachment #8572427 - Flags: feedback?(jaws)
Attachment #8572428 - Flags: feedback?(jaws)
Comment on attachment 8572428 [details] [diff] [review]
230924.diff

Review of attachment 8572428 [details] [diff] [review]:
-----------------------------------------------------------------

Just noticed something here before marking this as r+,

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +124,5 @@
> +    // If the properties are sorted, the next one will be 10.
> +    is(keyIterator.next().value, "10", "Third key is not 10");
> +    // If sorted next properties should be "abc" then "hello"
> +    is(keyIterator.next().value, "abc", "Fourth key is not abc");
> +    is(keyIterator.next().value, "hello", "Fifth key is not hello");

The descriptions for the last three test-cases all say "is not *" but it should be "is *"
Attachment #8572428 - Flags: feedback?(jaws) → feedback+
Attached patch 230924.diff (obsolete) — Splinter Review
Fixed some english in tests.
Attachment #8572428 - Attachment is obsolete: true
Attachment #8572435 - Flags: review?(jaws)
Attachment #8572435 - Flags: review?(jaws) → review+
Attachment #8572435 - Attachment is obsolete: true
Attachment #8572589 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/a4a679316000
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a4a679316000
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
The patch landing to Firefox Nightly and works fine.
Thanks!
Duplicate of this bug: 1146798
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.