Closed Bug 1244916 Opened 4 years ago Closed 3 years ago

JSON Viewer: empty arrays should hide the zero count


(DevTools :: JSON Viewer, defect)

Not set


(firefox50 fixed)

Firefox 50
Tracking Status
firefox50 --- fixed


(Reporter: clarkbw, Assigned: dalimil)




(1 file, 1 obsolete file)

Helen, what do you think about hiding the size count when the size is zero?  

via bug 1223143 comment 7

> ## JSON tab - misleading representation of empty arrays
> Empty arrays are represented as:
> key_name [0]
> (or "key_name: [0]" in object summaries)
> This, and the syntax coloring used (the zero is green, like a number literal
> in the Console) suggests that the value for key_name is an array with one
> value, the number 0. Yet the value is an *empty* array.
> It should probably be represented as "key_name: []".
> For the record, empty objects are: "key_name { }".

I wouldn't characterize the 0 as misleading, however it does seem superfluous given the colouring and that an empty array would give just as much meaning without confusing the eye.
Summary: JSON VIewer: empty arrays should hide the zero count → JSON Viewer: empty arrays should hide the zero count
Blocks: 1243951
Attached patch rev1 (obsolete) — Splinter Review
I made a patch that removes the array length from the brackets when it's equal to 0.
Assignee: nobody → dalimilhajek
Attachment #8770156 - Flags: review?(clarkbw)
Comment on attachment 8770156 [details] [diff] [review]

Awesome work!
I'm going to re-assign this to Honza who can do the review.
Attachment #8770156 - Flags: review?(clarkbw) → review?(odvarko)
Lin, I like this and it's also inline with how the Console works now. Just one little thing.

The console displays two spaces between the brackets:

Logging: `[]` produces: `Array [  ]`

I think there shouldn't be space at all (Chrome works that way too, ah and no `Array` title):

Logging: `[]` produces: `[]`

What do you think?
Flags: needinfo?(lclark)
This sounds good to me.
Flags: needinfo?(lclark)
Comment on attachment 8770156 [details] [diff] [review]

Review of attachment 8770156 [details] [diff] [review]:

Thanks for the patch!

Please see my inline comments.


::: devtools/client/shared/components/reps/array.js
@@ +127,5 @@
>        let items;
>        if (mode == "tiny") {
> +        let isEmpty = object.length === 0;
> +        items = DOM.span({className: "length"}, isEmpty ? "" : object.length);

The same thing needs to be done also for GripArray rep (grip-array.js)

We have two since, there are two kinds of reps:
1) For JS objects that are directly accessible
2) For remote JS objects that are accessible through RDP/grips

And I guess, tests also need to be adopted. There are two tests:


Attachment #8770156 - Flags: review?(odvarko)
Attached patch rev2Splinter Review
I changed the grip-array.js and the tests. Although I am still not quite sure where I can see the grip-array being used in the browser...
Attachment #8770156 - Attachment is obsolete: true
Attachment #8771437 - Flags: review?(odvarko)
Comment on attachment 8771437 [details] [diff] [review]

Review of attachment 8771437 [details] [diff] [review]:


> Although I am still not quite
> sure where I can see the grip-array being used in the browser...

E.g. in the DOM panel (can be enabled in the Options panel)

Attachment #8771437 - Flags: review?(odvarko) → review+
Keywords: checkin-needed
Pushed by
JSON Viewer: empty arrays should hide the zero count. r=odvarko
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.