Closed Bug 1347827 Opened 7 years ago Closed 7 years ago

Move the "persist logs" option to the netmonitor panel directly to make it more discoverable

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(platform-rel +, firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
platform-rel --- +
firefox57 --- fixed

People

(Reporter: pbro, Assigned: brennan.brisad, Mentored)

Details

(Whiteboard: [platform-rel-Zalando])

Attachments

(2 files, 2 obsolete files)

We have received this feedback several times in the past (see this 3 years old bug for instance: bug 1050190, but I also got this feedback via email too, and :Honza tells me he's gotten it several times too):

The way to persist network requests in the netmonitor isn't easily discoverable.
Right now, one needs to go in the options panel and enable it there.

Furthermore, it affects 2 panels: the console and the network. Bug 1346300 mentions that this is confusing.

Maybe this needs to be a meta bug with several sub bugs, but I think the following should be done:
- split the network and console requests/logs persisting features into 2 distinct features,
- move each of them to their respective panels, within a settings menu dropdown, like some panels do (cog icon in tools' toolbars).

Splitting the feature into 2 doesn't mean we necessarily need to get rid of the global setting that persists both console and requests logs at the same time. If we think it's important to keep, we could still have the checkbox in the options panel, rephrase the label so it's obvious what it does, and make it flip the 2 prefs at once.
platform-rel: --- → +
Whiteboard: [platform-rel-Zalando]
Thanks for the report! This would be great to have finally fixed, setting P2 for it.

Honza
Priority: -- → P2
Hi!
Can I work on this bug?
Sure, assigned to you, thanks!

Honza
Assignee: nobody → brennan.brisad
Attached patch bug1347827.patch (obsolete) — Splinter Review
Thanks!

Here I've made two new options menus, one for each panel.  On my system, when I open the menus they will extend outside the browser window if it isn't maximized.  This does not happen with the options menu for the performance panel.  I don't know if this is a problem or not.

I haven't done anything for the original checkbox in the toolbox options (currently it won't change automatically when the setting is modified in the console panel).  Should it be kept?  And in that case, how should it represent the case where one of the settings is enabled but not the other?  It might be clearer to remove it or add a new checkbox for the new setting.

Also, no test yet since it's only for feedback.
Attachment #8858938 - Flags: feedback?(odvarko)
Comment on attachment 8858938 [details] [diff] [review]
bug1347827.patch

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

This is great progress! Please see my comments below.

Honza

::: devtools/client/locales/en-US/netmonitor.properties
@@ +63,5 @@
>  expandDetailsPane=Show request details
>  
> +# LOCALIZATION NOTE (showOptionsMenu): This is the tooltip for the button
> +# that shows the netmonitor options menu in the UI.
> +showOptionsMenu=Show network monitor options

The Performance panel is using "Configure performance preferences". We should use the same wording to be consistent.


Also: all netmonitor strings should use `netmonitor.` prefix. So, the key in this  case should be something like: `netmonitor.toolbar.showOptionsMenu`

@@ +813,5 @@
>  netmonitor.headers.learnMore=Learn More
>  
> +# LOCALIZATION NOTE (netmonitor.options.persistRequests): This is the label displayed
> +# on the netmonitor options menu that toggles the persist requests setting.
> +netmonitor.options.persistRequests=Don't clear requests on navigation

We should use the existing label: "Enable persistent logs"

@@ +818,4 @@
>  
> +# LOCALIZATION NOTE (netmonitor.options.persistRequests.accesskey): This is the access key
> +# for the persist requests menu item displayed in the netmonitor options menu.
> +netmonitor.options.persistRequests.accesskey=C

So, the access key needs to change too (since the label is different)

::: devtools/client/locales/en-US/webconsole.properties
@@ +299,5 @@
> +
> +# LOCALIZATION NOTE (webconsole.optionsMenuButton.tooltip)
> +# Label used for the tooltip on the options menu button in the console top toolbar bar.
> +# Clicking on it will open the options menu.
> +webconsole.optionsMenuButton.tooltip=Show console options

The Performance panel is using "Configure performance preferences". We should use the same wording to be consistent.

@@ +304,5 @@
> +
> +# LOCALIZATION NOTE (webconsole.options.persistlog.label)
> +# Label used for an options-menu item. Clicking on it will toggle the persistence of
> +# logs across requests.
> +webconsole.options.persistlog.label=Persist logs across requests

We should stick to: "Enable persistent logs"

::: devtools/client/themes/common.css
@@ +373,5 @@
>  }
>  
> +.devtools-button.devtools-options-icon::before {
> +  background-image: var(--tool-options-image);
> +}

If you dock the Toolbox to the right side the Options button is aligned to the left. It should be aligned to the right. Perhaps it's related to bug 1357964 ?

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js
@@ +115,5 @@
> +          let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
> +          menu.popup(rect.left + screenX, rect.bottom + screenY, this.toolbox);
> +
> +          return menu;
> +        }

Any chance we could share the code that is responsible for the Menu and it's open+placement on the screen?
Attachment #8858938 - Flags: feedback?(odvarko) → feedback+
(In reply to Michael Brennan from comment #4)
> Here I've made two new options menus, one for each panel.  On my system,
> when I open the menus they will extend outside the browser window if it
> isn't maximized.  This does not happen with the options menu for the
> performance panel.  I don't know if this is a problem or not.
Can we share the implementation with the Performance panel?

> I haven't done anything for the original checkbox in the toolbox options
> (currently it won't change automatically when the setting is modified in the
> console panel).  Should it be kept?  And in that case, how should it
> represent the case where one of the settings is enabled but not the other? 
> It might be clearer to remove it or add a new checkbox for the new setting.
Yes, either remove the existing checkbox or append new. I would just remove
it to avoid duplication.

Honza
Blake, this bug report is about "Enable persistent log" option that is currently available in the global Options panel and affects two panels: Console and Network. To goal here to split this option into two options, so each panel has it's own and independent way to persist logs across page reloads.

Couple of questions:

1) It's been requested many times that options should be accessible directly from within related panel (not being hidden and hard to find somewhere else). This patch introduces a new button (with little gear icon) at the far right side of the Network and Console panel's toolbar. This is consistent with what the Performance panel already does. Does that sound ok? 

2) The tooltip of that button in Performance panel says: "Configure performance preferences". Should we keep the same wording to be consistent, i.e. "Configure network preferences" and "Configure console preferences"? Frankly, I would just use "Show options" everywhere.

3) Since the "Enable persistent log" options will be available directly in the Network and Console panels, I think we should remove it from the global Options panel to avoid duplication (and be consistent with what the Performance panel does). Does that sound ok?

Honza
Flags: needinfo?(bwinton)
These all seem like bigger questions than I can just answer off the top of my head, so I'm adding them to the big prioritized list o' UX questions, and will get a reply back to you when we can.  (Although I will say that Firefox as a whole is moving towards fewer places to configure stuff, not more.)
Flags: needinfo?(bwinton)
Thanks for your feedback!

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> If you dock the Toolbox to the right side the Options button is aligned to
> the left. It should be aligned to the right. Perhaps it's related to bug
> 1357964 ?

What about the "Filter URLs" input box and the request details button, should they also be right aligned or stay to the left?  I think the bug you refer to is unrelated.  To me it seems that the point of the bug is that the button icon is wrong, instead of showing left and right arrows it should change to up and down.  Not 100% sure though.

> :::
> devtools/client/webconsole/new-console-output/new-console-output-wrapper.js
> @@ +115,5 @@
> > +          let screenY = target.ownerDocument.defaultView.mozInnerScreenY;
> > +          menu.popup(rect.left + screenX, rect.bottom + screenY, this.toolbox);
> > +
> > +          return menu;
> > +        }
> 
> Any chance we could share the code that is responsible for the Menu and it's
> open+placement on the screen?

Yeah, I'll move the logic into the options-menu.js file.
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to Michael Brennan from comment #4)
> > Here I've made two new options menus, one for each panel.  On my system,
> > when I open the menus they will extend outside the browser window if it
> > isn't maximized.  This does not happen with the options menu for the
> > performance panel.  I don't know if this is a problem or not.
> Can we share the implementation with the Performance panel?

I didn't think so as the Performance panel defines the menu in `performance.xul`.  Is it possible?

> 
> > I haven't done anything for the original checkbox in the toolbox options
> > (currently it won't change automatically when the setting is modified in the
> > console panel).  Should it be kept?  And in that case, how should it
> > represent the case where one of the settings is enabled but not the other? 
> > It might be clearer to remove it or add a new checkbox for the new setting.
> Yes, either remove the existing checkbox or append new. I would just remove
> it to avoid duplication.

Yes I agree.
Attached image layout.png
(In reply to Michael Brennan from comment #9)
> What about the "Filter URLs" input box and the request details button,
> should they also be right aligned or stay to the left?  I think the bug you
> refer to is unrelated.  To me it seems that the point of the bug is that the
> button icon is wrong, instead of showing left and right arrows it should
> change to up and down.  Not 100% sure though.
You are right, it's about the icon direction.

I am attaching a screenshot. It shows how the filter-url input, details and options buttons are nicely aligned to the right if there is enough space and it fits on one line (together with the filter buttons). In case the filter-url input is wrapped to separate line (the docked toolbox is thinner) it's aligned to the left. I think it should still on the right side - even if on separate line. It's ok for me if this is done in separate bug since it's separate issue (and it might need some more discussion).

(In reply to Michael Brennan from comment #10)
> (In reply to Jan Honza Odvarko [:Honza] from comment #6)
> > (In reply to Michael Brennan from comment #4)
> > > Here I've made two new options menus, one for each panel.  On my system,
> > > when I open the menus they will extend outside the browser window if it
> > > isn't maximized.  This does not happen with the options menu for the
> > > performance panel.  I don't know if this is a problem or not.
> > Can we share the implementation with the Performance panel?
> 
> I didn't think so as the Performance panel defines the menu in
> `performance.xul`.  Is it possible?
Ah, I see, Than we need to have our own implementation.

In any case, we need to wait for Blake's response (see comment#8) to move further with this bug.

Honza
Flags: needinfo?(odvarko)
A response to the description / comment#0 , specifically:
"Splitting the feature into 2 doesn't mean we necessarily need to get rid of the global setting that persists both console and requests logs at the same time. If we think it's important to keep, we could still have the checkbox in the options panel, rephrase the label so it's obvious what it does, and make it flip the 2 prefs at once."

Currently "enable persistent logs" is a global setting that is shared between tabs. The above comment seems to imply that the toggles will split up for console/network, but will still be global settings.

In the other comments I've not seen any wording to imply this would be an

I've seen quite some slowdown when the console and network logs have grown quite a bit because persistent logging is on everywhere. I really preferred the way Firebug did this; a per-tab, per-functionality (console/network) setting. That way I can easily enable persistent logging when I need it, where I need it.


By the way, this seems to be a duplicate of bug #1145669.
Michael: if you are still interested in working on this, you should know the persist log preference was recently split in two : one preference for the console and one preference for the netmonitor. The console added a checkbox to their UI to toggle persist logs.

This means that this bug should now focus on the netmonitor. I see the current approach in this patch is to add an options menu. But maybe as a first step we could simply add a checkbox in UI in the same way as the console? It would be very nice to have this feature for both console and netmonitor in Firefox 57!
Flags: needinfo?(brennan.brisad)
(In reply to Julian Descottes [:jdescottes] from comment #13)
> Michael: if you are still interested in working on this, you should know the
> persist log preference was recently split in two : one preference for the
> console and one preference for the netmonitor. The console added a checkbox
> to their UI to toggle persist logs.
> 
> This means that this bug should now focus on the netmonitor. I see the
> current approach in this patch is to add an options menu. But maybe as a
> first step we could simply add a checkbox in UI in the same way as the
> console? It would be very nice to have this feature for both console and
> netmonitor in Firefox 57!

Hi Julian,
I'm still interested! :) I'd be happy to add the checkbox for the NetMonitor.

Do we no longer need to wait for the response from UX then, as mentioned in comment #11?
Flags: needinfo?(brennan.brisad) → needinfo?(jdescottes)
Great, thanks for replying so quickly! I had totally missed that this bug was pending a UX review :)

Let's check with Victoria. Since the console went ahead and already added a checkbox on their side, I think the most natural thing to do for the moment is to align with them and to add a similar checkbox in the netmonitor UI.

We can always come up with a better UI for devtools options later!
Flags: needinfo?(jdescottes) → needinfo?(victoria)
Ah yes! Eventually I will work on a consistent per-panel settings UI, but for now, sounds good to add a Persist Logs checkbox into the Network bar with the other filter settings. 

I think it would make most sense to put the Persist Logs checkbox next to the Disable Cache checkbox (whichever is more important should be on the left).
Flags: needinfo?(victoria)
I'm working on it :)
Attached patch bug1347827.patch (obsolete) — Splinter Review
Here's a patch which adds a toolbar checkbox like the one in webconsole.
I also removed the setting from the toolbox options.
Attachment #8858938 - Attachment is obsolete: true
Attachment #8906317 - Flags: review?(odvarko)
Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d4eecfeaafdaf3f931f8558e2ecb5dd91da384

(but I am not sure what is a good set of try settings, so I just guessed)
Comment on attachment 8906317 [details] [diff] [review]
bug1347827.patch

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

Thanks for the patch, works great!

Just a few comments related to eslint

* toggleRequestFilterType should be placed after componentWillUnmount

R+ assuming my comments are fixed and try is green.

One more try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9dd875ec3612d3b279c5b53294b6fc41cc7b5e0

Honza

::: devtools/client/netmonitor/src/components/toolbar.js
@@ +79,1 @@
>        toggleBrowserCache,

both these props need to be in propTypes (for validation)

@@ +182,5 @@
>    },
>  
> +  updatePersistentLogsEnabled() {
> +    this.props.enablePersistentLogs(
> +      Services.prefs.getBoolPref(DEVTOOLS_ENABLE_PERSISTENT_LOG_PREF));

enablePersistentLogs also needs to be in propTypes
Attachment #8906317 - Flags: review?(odvarko) → review+
Thanks for the review!

(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> Just a few comments related to eslint
> 
> * toggleRequestFilterType should be placed after componentWillUnmount

Does eslint report this?  `./mach eslint` just gave me a couple of unrelated errors in other files.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfe6ff24019
Add Persist Logs checkbox in NetMonitor toolbar. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1cfe6ff24019
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.