Closed Bug 1247064 Opened 8 years ago Closed 8 years ago

JSON Viewer: auto-expand tree view

Categories

(DevTools :: JSON Viewer, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: clarkbw, Assigned: Honza)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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
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 :)
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
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
(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
Attached patch bug1247064.patch (obsolete) — Splinter Review
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.
Blocks: 1251949
(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
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+
Attached patch bug1247064.patch (obsolete) — Splinter Review
(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+
Attached patch bug1247064.patch (obsolete) — Splinter Review
Yet fixing the commit message

Honza
Attachment #8741717 - Attachment is obsolete: true
Attachment #8741729 - Flags: review+
Attached patch bug1247064.patchSplinter Review
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+
The try looks good to me (some oranges, but unrelated)

Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15a5be62efb9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1370139
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.