JSON Viewer: auto-expand tree view

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: JSON Viewer
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: clarkbw, Assigned: Honza)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

coming out of bug 1234511

I believe there are two user stories here so this bug might need to be broken down further. 

User Stories

1) As a web developer using the JSON Viewer I would like the viewer to default to expanding the attributes instead of collapsing them such that when I load a JSON document I don't need to begin expanding all the items to see their contents.

2) As a web developer using the JSON Viewer I would like the viewer to expand sub-trees of data as I expand the parent level items such that I don't need tediously to expand each and every level to view their contents.
(Reporter)

Comment 1

2 years ago
Just for reference here are the previous comments:

(armisael from bug 1234511 comment #7)
> > > - Everything is closed by default so I have to click multiple time to open
> > > all child
> > 
> > Do JsonView auto-expand everything or is there some kind of algorithm it
> > uses to not go too deep?
> 
> Let me jump in: an "expand-all" button would be great; but if possible, I
> would also use a simple algorithm to auto-expand the document: expand the
> items until the json fills the whole window, then stop. I don't know if
> that's possible, but for sure it's practical: there's no need to keep all
> that empty space, but expanding everything by default may be as wrong as
> having everything closed by default.
> 
> About expanding items automatically, it would be great if it expanded items
> also when searching; there's a discussion going on on this already in bug
> 1217131
Duplicate of this bug: 1248880
Yes please! I rarely benefit from the collapsed view. My ability to scroll the page is much faster than the benefit of seeing a list of the keys. All too often, I have to take the URL I'm looking at to the terminal and run `curl someurl | json_print | less`.
+1
Example:
Go to https://l10n.mozilla-community.org/~pascalc/stores_l10n/api/apple/translation/release/fr/

With the JsonView extension I can read everything immediately. With Firefox built-in json viewer, I need to click *17* times on widgets to see the same data.
JsonView is no longer available as an addon :(
I tried JSONovich but it doesn't seem to work. 

I've grown used to having to copy-and-paste the URL to Chrome or curl on the command line when I want to see JSON.
(In reply to Peter Bengtsson [:peterbe] from comment #5)
> JsonView is no longer available as an addon :(
> I tried JSONovich but it doesn't seem to work. 
> 
> I've grown used to having to copy-and-paste the URL to Chrome or curl on the
> command line when I want to see JSON.

JsonView is still available on AMO:
https://addons.mozilla.org/en-US/firefox/addon/jsonview/

If you want to use it on Nightly, you need to deactivate our own Json Viewer by setting this pref in about:config to false:
devtools.jsonview.enabled
(In reply to Pascal Chevrel:pascalc from comment #6)
> (In reply to Peter Bengtsson [:peterbe] from comment #5)
> > JsonView is no longer available as an addon :(
> > I tried JSONovich but it doesn't seem to work. 
> > 
> > I've grown used to having to copy-and-paste the URL to Chrome or curl on the
> > command line when I want to see JSON.
> 
> JsonView is still available on AMO:
> https://addons.mozilla.org/en-US/firefox/addon/jsonview/
> 
> If you want to use it on Nightly, you need to deactivate our own Json Viewer
> by setting this pref in about:config to false:
> devtools.jsonview.enabled

Wow! That is so helpful!
But we're getting off track. 

The only value of this hacking discussion (between Pascal and me) is to add weight to the importance getting this fixed :)
(Reporter)

Comment 8

2 years ago
We could use a decision on some kind of algorithm to try for expanding.  I believe auto-expanding all collapsed properties can lead to some poor performance issues.  Something like expanding the first 7 child objects of an expanded parent might be a good start to making this work for most of the people most of the time without freezing up the UI on some of the people some of the time.  We could iterate from there and see if 7 is the magic number or a miss.

i.e.

This:

[+] - parent 1

Becomes this with 1 click:

[-]+- parent 1
  [-]+- child 1
    [-]+- child 1.1
      [-]+- child 1.2
        [-]+- child 1.3
          [-]+- child 1.4
            [-]+- child 1.5
              [-]+- child 1.6
                [+]+- child 1.7 // not expanded further than this
  [+]-- child 2
  [+]-- child 3
  [+]-- child 4
  [+]-- child 5
  [+]-- child 6
  [+]-- child 7
  [+]-- child 8
  [+]-- child 9... // other children here would be expanded

And we might want to work on the default open status (user story #1) for the initial parent in another bug.
There might be value in differentiating between objects and arrays. It might make sense to always expand objects by default, but be more careful about expanding arrays.

(Also, I think there are some quick wins here. Maybe the smart perf-aware optimizations can be postponed to another bug in order to prevent the perfect becoming the enemy of the good?)
One option for perf might be to auto-expand documents (and arrays) which are under a certain size, in characters or bytes. I suspect that if someone gets a 1KB JSON response, they just want to read it, but a 200KB response, not so much. They'd be more likely to want to search it.

Also, whatever the default is, can we have some hotkeys for expanding/collapsing, like in Thunderbird for threads?

Gerv

Updated

2 years ago
Duplicate of this bug: 1248383

Comment 12

2 years ago
I would definitly look at the size or something to be honest I use Jsonview in Chrome / Firefox and never had a performance issue with it.
It would only be an issue on very very large documents.
diff -r dd1abe874252 devtools/client/jsonview/components/reps/tree-view.js
--- a/devtools/client/jsonview/components/reps/tree-view.js	Thu Mar 10 11:51:35 2016 +0100
+++ b/devtools/client/jsonview/components/reps/tree-view.js	Thu Mar 10 10:54:17 2016 -0800
@@ -217,7 +217,7 @@
     open: "",
     hasChildren: hasChildren,
     value: value,
-    open: false,
+    open: (uid < 50),  // auto-expand the first 50 items
     key: uid++
   };
 
Honza, what do you think about doing something like this?

I made this change and it handles the auto-expand case pretty well so far, the first 50 items created are automatically expanded and the rest, if there are more, default to be collapsed.  

Also, I have no idea why the open attribute is listed twice in the object, first as a string and then a boolean.
Flags: needinfo?(odvarko)
(In reply to Gervase Markham [:gerv] from comment #10)
> Also, whatever the default is, can we have some hotkeys for
> expanding/collapsing, like in Thunderbird for threads?

Stay on target... :)  I filed bug 1255828 for keyboard nav
(Assignee)

Updated

2 years ago
Depends on: 1256757
(Assignee)

Comment 15

2 years ago
(In reply to Bryan Clark (Firefox PM) [:clarkbw] from comment #13)
> diff -r dd1abe874252 devtools/client/jsonview/components/reps/tree-view.js
> --- a/devtools/client/jsonview/components/reps/tree-view.js	Thu Mar 10
> 11:51:35 2016 +0100
> +++ b/devtools/client/jsonview/components/reps/tree-view.js	Thu Mar 10
> 10:54:17 2016 -0800
> @@ -217,7 +217,7 @@
>      open: "",
>      hasChildren: hasChildren,
>      value: value,
> -    open: false,
> +    open: (uid < 50),  // auto-expand the first 50 items
>      key: uid++
>    };
>  
> Honza, what do you think about doing something like this?
Yes, we can do something like that.

Note that the JSON Viewer needs to use shared tree component from:
devtools/client/shared/components/tree
(this is covered by bug 1256757, wip)

Consequently we can customize that component using something
like mentioned above.

I'll get back to this as soon as bug 1256757 lands.
(keeping NI me)

Honza
(Assignee)

Comment 16

2 years ago
Created attachment 8733411 [details] [diff] [review]
bug1247064.patch

Still work in progress, but:
- The patch implements auto expand for documents < 100KB, max 7 level deep.
- I'll yet implement a toolbar-button that can be used to auto expand manually.

Feedback welcome already.

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Comment on attachment 8733411 [details] [diff] [review]
bug1247064.patch

This looks good!

(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> Created attachment 8733411 [details] [diff] [review]
> bug1247064.patch
> 
> Still work in progress, but:
> - The patch implements auto expand for documents < 100KB, max 7 level deep.

I think we should get this landed as is.

> - I'll yet implement a toolbar-button that can be used to auto expand
> manually.

Lets file another bug for the toolbar button.  We should get some help from Helen for that work but we can move forward with this auto expand for now.

Updated

2 years ago
Blocks: 1251949
(Assignee)

Updated

2 years ago
Blocks: 1264557
(Assignee)

Comment 18

2 years ago
(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #17)
> Lets file another bug for the toolbar button.  We should get some help from
> Helen for that work but we can move forward with this auto expand for now.
Agreed. I filled bug 1264557

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5ecc4920ca6

Honza
(Assignee)

Updated

2 years ago
Attachment #8733411 - Flags: review?(jryans)
Comment on attachment 8733411 [details] [diff] [review]
bug1247064.patch

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

Seems to work well overall.  Needs a real commit message.

::: devtools/client/jsonview/components/json-panel.js
@@ +105,5 @@
>        }];
>  
> +      // Expand the document by default if its size isn't bigger than 100KB.
> +      let expandedNodes = new Set();
> +      if (this.props.jsonText.length <= AUTO_EXPAND_MAX_SIZE) {

Since you only need the text length, why not pass that in, instead of the entire text itself?
Attachment #8733411 - Flags: review?(jryans) → review+
(Assignee)

Comment 20

2 years ago
Created attachment 8741717 [details] [diff] [review]
bug1247064.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> Comment on attachment 8733411 [details] [diff] [review]
> bug1247064.patch
> 
> Review of attachment 8733411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems to work well overall.  Needs a real commit message.
> 
> ::: devtools/client/jsonview/components/json-panel.js
> @@ +105,5 @@
> >        }];
> >  
> > +      // Expand the document by default if its size isn't bigger than 100KB.
> > +      let expandedNodes = new Set();
> > +      if (this.props.jsonText.length <= AUTO_EXPAND_MAX_SIZE) {
> 
> Since you only need the text length, why not pass that in, instead of the
> entire text itself?
Done

One test failure also fixed.

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e68b30a0090

Thanks Ryan for the review!
Honza
Attachment #8733411 - Attachment is obsolete: true
Attachment #8741717 - Flags: review+
(Assignee)

Comment 21

2 years ago
Created attachment 8741729 [details] [diff] [review]
bug1247064.patch

Yet fixing the commit message

Honza
Attachment #8741717 - Attachment is obsolete: true
Attachment #8741729 - Flags: review+
(Assignee)

Comment 22

2 years ago
Created attachment 8742203 [details] [diff] [review]
bug1247064.patch

Yet removing the expand-all button (will be done as part of bug 1264557)

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b30368b8ba82

Honza
Attachment #8741729 - Attachment is obsolete: true
Attachment #8742203 - Flags: review+
(Assignee)

Comment 23

2 years ago
The try looks good to me (some oranges, but unrelated)

Honza
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/15a5be62efb9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

9 months ago
Depends on: 1370139
You need to log in before you can comment on or make changes to this bug.