Closed Bug 1328828 Opened 4 years ago Closed 4 years ago

Implement Properties View for reusing in network details panels

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.4 - Jan 9
Tracking Status
firefox53 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(2 files)

A breakdown task from ParamsPanel a.k.a bug 1317650 for creating a shareable and customized TreeView component to be reused by Headers, Cookies, Params and Response panels.

- This component will import TreeView and SourceEditor. Append SourceEditor inside TreeView so that the SourceEditor can be collapsible.
- We can benefit from it like getting rid of duplicated methods getRowClass(), onFilter(), renderRow() and renderValue() in network details panels.
- Right now, I'd prefer to use PropertiesView as component name, but any suggestions would be appreciated.
It would be nice if the searchbox were included in it as well.
Duplicate of this bug: 1326036
Component: Developer Tools: Netmonitor → Developer Tools: Shared Components
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #1)
> It would be nice if the searchbox were included in it as well.

Nice suggestion!

I'm still thinking this component is not generic enough to reusable out of netmonitor, so it should be part of netmonitor bug. And I also found that those components in client/shared/components are designed for common purpose (reps, splitter, tabs and tree), so it would be better if we just put properties-view in net monitor/shared/.
Component: Developer Tools: Shared Components → Developer Tools: Netmonitor
Upload patch for PropertiesView component, and please note that this patch is unable to be tested without combining with ParamsPanel. So please apply this patch with https://bugzilla.mozilla.org/show_bug.cgi?id=1317650#c82 together to see how properties view works.
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]
Attached image source-editor.png
Honza found an issue in source editor https://bugzilla.mozilla.org/show_bug.cgi?id=1328500#c13.

Right now, source editor is unable to take the rest of the vertical available space. We should find out a way to work it out.
Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103208

::: devtools/client/netmonitor/shared/components/properties-view.js:100
(Diff revision 2)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());
> +}
> +
> +function renderRow(props) {

I think it'd be a better approach to pass in renderRow as prop to this component, since the current one is quite specific to the Params panel.

::: devtools/client/netmonitor/shared/components/properties-view.js:118
(Diff revision 2)
> +  if (typeof value === "object" &&
> +      (open || !path.includes(L10N.getStr("jsonScopeName")))) {
> +    return null;
> +  }

This check could be a function to pass in as prop:

if (!props.shouldRenderValue({value, path}))
  return null;
}

The component can be more generic like this.
(In reply to Ricky Chien [:rickychien] from comment #7)
> Created attachment 8824302 [details]
> source-editor.png
> 
> Honza found an issue in source editor
> https://bugzilla.mozilla.org/show_bug.cgi?id=1328500#c13.
> 
> Right now, source editor is unable to take the rest of the vertical
> available space. We should find out a way to work it out.

I'm investigating using flexbox (or any better alternative) for the treetable in bug 1329068, since table layouts are really hard to manipulate.
Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103208

> This check could be a function to pass in as prop:
> 
> if (!props.shouldRenderValue({value, path}))
>   return null;
> }
> 
> The component can be more generic like this.

I'm not going to change to props.shouldRenderValue() here but use if (props.member.level === 0) to hide first level rep summary instead.
Depends on: 1328500
Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103392

Looks good, thanks. I have just a few comments.

::: devtools/client/netmonitor/shared/components/editor.js:40
(Diff revision 4)
> +      lineNumbers: true,
> +      readOnly: true,
> +      value: text,
> +    });
> +
> +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);

It would be nice if we could use appendToLocalElement here, just like debugger.html does. It would not create a special iframe for the editor, and no async code with promises would be needed.

I'm pretty sure that the problems with appendToLocalElement are caused by XUL. If you look at the editor elements created by appendToLocalElement, they are divs and spans, but in the XUL namespace, not HTML.

Please file a followup bug, so that we don't forget to fix this after netmonitor.xul is fully converted to HTML.

::: devtools/client/netmonitor/shared/components/properties-view.js:41
(Diff revision 4)
> +  filterText = "",
> +  filterPlaceHolder = "",
> +  onFilterChange,

This component would be more reusable and easy to use if the filterText/onFilterChange logic didn't go out to Redux and back, but was in the local component state instead.

::: devtools/client/netmonitor/shared/components/properties-view.js:103
(Diff revision 4)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());

Is bug 1325930 really relevant here? When I debug the code, I see that there is no JS object that would have "name" and "value" properties - there are exactly the data that were in the request params.
Attachment #8823990 - Flags: review?(jsnajdr) → review+
Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103450

::: devtools/client/netmonitor/shared/components/editor.js:40
(Diff revision 4)
> +      lineNumbers: true,
> +      readOnly: true,
> +      value: text,
> +    });
> +
> +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);

Thanks for finding the root cause! I'll file bug for getting rid of deferEditor and use appendToLocalElement instead.

::: devtools/client/netmonitor/shared/components/properties-view.js:103
(Diff revision 4)
> +  // FIXME: Get rid of name and value properties due to bug 1325930
> +  let jsonString = JSON.stringify({ [name]: value }).toLowerCase();
> +  return jsonString.includes(filterText.toLowerCase());

Yes, bug 1325930 is a similar bug in JSON View.

The object parameter passes in onFilter will contain name and value properties, which is incorrect.

Ah, could you check it again? I still see { name: xxx, value: xxx } when inserting console.log(object)
(In reply to Jarda Snajdr [:jsnajdr] from comment #13)
> Comment on attachment 8823990 [details]
> Bug 1328828 - Implement Properties View
> 
> https://reviewboard.mozilla.org/r/102498/#review103392
> 
> Looks good, thanks. I have just a few comments.
> 
> ::: devtools/client/netmonitor/shared/components/editor.js:40
> (Diff revision 4)
> > +      lineNumbers: true,
> > +      readOnly: true,
> > +      value: text,
> > +    });
> > +
> > +    this.deferEditor = this.editor.appendTo(this.refs.editorElement);
> 
> It would be nice if we could use appendToLocalElement here, just like
> debugger.html does. It would not create a special iframe for the editor, and
> no async code with promises would be needed.
> 
> I'm pretty sure that the problems with appendToLocalElement are caused by
> XUL. If you look at the editor elements created by appendToLocalElement,
> they are divs and spans, but in the XUL namespace, not HTML.
> 
> Please file a followup bug, so that we don't forget to fix this after
> netmonitor.xul is fully converted to HTML.

Bug 1329211 has filed for addressing this problem.
Comment on attachment 8823990 [details]
Bug 1328828 - Implement Properties View

https://reviewboard.mozilla.org/r/102498/#review103626

I like the patch. Just a few inline comments.


Honza

::: devtools/client/netmonitor/shared/components/editor.js:16
(Diff revision 5)
> +
> +/**
> + * CodeMirror editor as a React component
> + */
> +const Editor = createClass({
> +

nit: please remove this empty line

::: devtools/client/netmonitor/shared/components/properties-view.js:38
(Diff revision 5)
> + * Source editor - Enable by specifying object's 1 level property name to "editorText".
> + * Rep - Default enabled.
> + */
> +const PropertiesView = createClass({
> +
> +  displayName: "PropertiesView",

nit: please remove the empty line

::: devtools/client/netmonitor/shared/components/properties-view.js:74
(Diff revision 5)
> +    let filterText = this.state.filterText;
> +
> +    if (!filterText || whiteList.includes(name)) {
> +      return true;
> +    }
> +    // FIXME: Get rid of name and value properties due to bug 1325930

Note that bug 1325930 has been fixed so, changes here need to reflect that.

::: devtools/client/netmonitor/shared/components/properties-view.js:154
(Diff revision 5)
> +            enableInput,
> +            expandableStrings,
> +            expandedNodes: new Set(sectionNames.map((sec) => "/" + sec)),
> +            onFilter: (props) => this.onFilter(props, sectionNames),
> +            renderRow: renderRow || this.renderRowWithEditor,
> +            renderValue: renderValue || this.renderValueWithRep,

I am thinking if the support for 'enableInput' shouldn't be part of this component and not the generic tree. Or do you think it can be used somewhere outside of the network panel?

::: devtools/client/themes/netmonitor.css:1175
(Diff revision 5)
> +.tree-container .treeTable .treeRow.tree-section > .treeLabelCell > .treeLabel:hover {
> +  color: var(--theme-body-color-alt);
> +}
> +
> +.tree-container .treeTable .treeValueCell {
> +  /* FIXME: Make value cell can be reduced to shorter witdh */

typo: witdh -> width
Attachment #8823990 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> I am thinking if the support for 'enableInput' shouldn't be part of this
> component and not the generic tree. Or do you think it can be used somewhere
> outside of the network panel?

Input filed is able to benefit to user when they want to select a long value in TreeView, so we can have this feature built-in TreeView component so that developer can enable this feature easily. In short, I have no idea how many use cases outside of network panel will need this feature, but it's still a good option to people who needs to deal with very long string in TreeCell.

I'm ok if we want TreeView to be more generic and get rid of `enableInput` in TreeView, but someone must take responsibility for supporting it anyway, the candidate might be PropertiesView itself.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2f372cf7314
Implement Properties View r=Honza,jsnajdr
(In reply to Ricky Chien [:rickychien] from comment #20)
> I'm ok if we want TreeView to be more generic and get rid of `enableInput`
> in TreeView, but someone must take responsibility for supporting it anyway,
> the candidate might be PropertiesView itself.
Yep, this is precisely what I mean. No big priority but, we might do in at some point.

Honza
Blocks: 1329575
Flags: qe-verify? → qe-verify-
https://hg.mozilla.org/mozilla-central/rev/c2f372cf7314
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.