Closed
Bug 1247064
Opened 9 years ago
Closed 9 years ago
JSON Viewer: auto-expand tree view
Categories
(DevTools :: JSON Viewer, defect)
DevTools
JSON Viewer
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: clarkbw, Assigned: Honza)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
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•9 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
Comment 3•9 years ago
|
||
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`.
Comment 4•9 years ago
|
||
+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.
Comment 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
(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
Comment 7•9 years ago
|
||
(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•9 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.
Comment 9•9 years ago
|
||
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?)
Comment 10•9 years ago
|
||
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
Comment 12•9 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.
Reporter | ||
Comment 13•9 years ago
|
||
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)
Reporter | ||
Comment 14•9 years ago
|
||
(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 | ||
Comment 15•9 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•9 years ago
|
||
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)
Reporter | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 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•9 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•9 years ago
|
||
(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•9 years ago
|
||
Yet fixing the commit message Honza
Attachment #8741717 -
Attachment is obsolete: true
Attachment #8741729 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
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•9 years ago
|
||
The try looks good to me (some oranges, but unrelated) Honza
Keywords: checkin-needed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/15a5be62efb9
Keywords: checkin-needed
Comment 25•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/15a5be62efb9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•