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)
DevTools
Netmonitor
Tracking
(platform-rel +, firefox57 fixed)
RESOLVED
FIXED
Firefox 57
People
(Reporter: pbro, Assigned: brennan.brisad, Mentored)
Details
(Whiteboard: [platform-rel-Zalando])
Attachments
(2 files, 2 obsolete files)
46.72 KB,
image/png
|
Details | |
18.63 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
platform-rel: --- → +
Whiteboard: [platform-rel-Zalando]
Comment 1•7 years ago
|
||
Thanks for the report! This would be great to have finally fixed, setting P2 for it. Honza
Priority: -- → P2
Updated•7 years ago
|
Mentor: odvarko
Assignee | ||
Comment 2•7 years ago
|
||
Hi! Can I work on this bug?
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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+
Comment 6•7 years ago
|
||
(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
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Comment 11•7 years ago
|
||
(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)
Comment 12•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Comment 15•7 years ago
|
||
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)
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
I'm working on it :)
Assignee | ||
Comment 18•7 years ago
|
||
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)
Assignee | ||
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
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+
Assignee | ||
Comment 21•7 years ago
|
||
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.
Assignee | ||
Comment 22•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf1d10957d357f85faeb5169387e4920716c00e
Attachment #8906317 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 23•7 years ago
|
||
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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1cfe6ff24019
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•