1307881 - Add a checkbox in the filter UI to toggle message persistence

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Lin Clark [:linclark]]

Originally posted by:linclark

see https://github.com/devtools-html/gecko-dev/issues/131

Depends on #101 

The persist logs option allows you to keep your messages even when you refresh/navigate the page. We need to do a few things:

1. Make a checkbox in the filter bar
2. Persist the choice using prefs
3. Add handling for navigation messages coming from the backend
4. Clear messages on navigation unless the checkbox is checked

Eventually, we will also remove the existing checkbox from settings (which will happen in #106)

Comment 1

[Brian Grinstead [:bgrins]]

Discussed and we can use this bug to remove the checkbox from the settings as well

Comment 2

[Brian Grinstead [:bgrins]]

Scoping this bug to the filter UI change, since message persistence is already working

Comment 4

[Brian Grinstead [:bgrins]]

Honza, do you have an opinion about splitting up the the message persistence option for the webconsole and netmonitor?  Right now they both key on "devtools.webconsole.persistlog" (which is set in the Common Preferences section in the options panel).  If we surface the option in the console UI then it would be inconsistent to also change netmonitor persistence when that option changes.

I'm inclined to do it like this:

1) Remove the Common Preferences section in the toolbox options (this is the only checkbox in that section right now)
2) Add a checkbox for "Preserve logs" in the Webconsole UI. This would likely be inside the hidden-by-default filter area, but ni? Victoria to see if she has opinions about where to put this tool specific setting
3) Add a checkbox for "Preserve logs" either in the netmonitor UI or in the toolbox settings. Again, I guess this would go next to the 'disable cache' checkbox there, but we are tight on space. If we don't have a good place to put it, we could stick it in the toolbox settings in a new Netmonitor section. Note that we've talked in the past about making a consistent UI across tools for putting settings specific to that tool but we haven't come up with anything

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> 1) Remove the Common Preferences section in the toolbox options (this is the
> only checkbox in that section right now)
> 2) Add a checkbox for "Preserve logs" in the Webconsole UI. This would
> likely be inside the hidden-by-default filter area, but ni? Victoria to see
> if she has opinions about where to put this tool specific setting
> 3) Add a checkbox for "Preserve logs" either in the netmonitor UI or in the
> toolbox settings. Again, I guess this would go next to the 'disable cache'
> checkbox there, but we are tight on space.
Yes, exactly, the space on the toolbar is expensive.

I was always thinking about having specific option menu for every panel
accessible directly from panel's toolbar. It could be e.g. at the far
right side - the Performance panel already does that.

Reasoning behind this:
- Users don't know about the global Toolbox option panel
- It feels better to have UI for panel options directly accessible from within the panel
- We don't have to mix all our options in one big heap of options.

In any case, I agree with splitting the persistence option, so the Console and the Network
panel maintain their own pref & state. There, might be some steps in implementation, e.g.
when Console has its own option the one in the Toolbox options could be used only
by the Net (till Net has its own option at the right UI place).

Honza


> If we don't have a good place to
> put it, we could stick it in the toolbox settings in a new Netmonitor
> section. Note that we've talked in the past about making a consistent UI
> across tools for putting settings specific to that tool but we haven't come
> up with anything

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Style Editor

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Performance

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

We already have such things for Style editor (attachment 8895742 [details]) and the Performance panel (attachment 8895743 [details]).
They are not perfect though, maybe we could do the console one like the photon menus (hamburger, library, …).

Comment 9

[Victoria Wang [:victoria]]

(Clearing the needinfo) I talked to bgrins about the need for a unified UI for tool-specific prefs - I'll be looking into that as part of the photon refresh. For now I think it's a good idea to move forward with your tentative plans.

Comment 10

[Brian Grinstead [:bgrins]]

Sounds like some new separator styling will be added in Bug 1394513. Doesn't necessarily block this work but would be a way to provide visual separation between the various severties, categories, and the persist checkbox

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1307881 - Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;

Review commit: https://reviewboard.mozilla.org/r/173480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173480/

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

Review commit: https://reviewboard.mozilla.org/r/173482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173482/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

Just want some feedback before proceeding on tests here. The part I'm not sure about is if this should be managed in PrefState or UiState. I originally used UiState just because we have a similar toggle feature for the filter bar and I was using that as a basis for all the boilerplate. But now I'm wondering if we should instead manage this in PrefState and map it out into the props on the UI. We haven't really used PrefState for much yet (there's only one non changeable pref that never makes its way into the UI) so wanted to run this by you - what do you think?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

https://reviewboard.mozilla.org/r/173482/#review178932

::: devtools/client/webconsole/new-console-output/components/filter-bar.js:172
(Diff revision 1)
> +      FilterCheckbox({
> +        label: l10n.getStr("webconsole.enablePersistentLogs.label"),
> +        title: l10n.getStr("webconsole.enablePersistentLogs.tooltip"),
> +        onChange: this.onChangePersistToggle,
> +        checked: persistLogs,
>        })

I thought we wanted to display it in the top bar, not the filter one ?
If we put it here, maybe we should change the icon (as well as the tooltip) of the button to toggle the visibility of this sub-toolbar.

At the moment it says "Toggle the filter bar", which I'm not sure would make sense anymore with the persist checkbox in it.

Maybe we could simply have a cog icon, put it on the right of the top-toolbar, and have the title say "Toggle the console settings panel" or something alike ?

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

> (In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8902070 [details]
> Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing
> persistence
> 
> Just want some feedback before proceeding on tests here. The part I'm not
> sure about is if this should be managed in PrefState or UiState. I
> originally used UiState just because we have a similar toggle feature for
> the filter bar and I was using that as a basis for all the boilerplate. But
> now I'm wondering if we should instead manage this in PrefState and map it
> out into the props on the UI. We haven't really used PrefState for much yet
> (there's only one non changeable pref that never makes its way into the UI)
> so wanted to run this by you - what do you think?

I think the idea behind the pref state was having a way to reflect changes made directly in the prefs (e.g. in about:config) in the UI. But if I remember correctly we are not doing this at the moment.

I'd say that the "correct" workflow would be: 
- Clicking on the checkbox changes the pref
- A listener on prefs changes trigger a Pref action, which will change the Pref state
- The checkbox state then reflects the Pref state

With this, setting the prefs in about:config or in the global toolbox settings would change the checkbox UI.

That being said, I'm fine with this patch, since I guess it shouldn't be common to change this pref directly in about:config (and I guess we'll remove the checkbox in the toolbox settings panel).

We could have it done that way for now, and have a follow up later to say that the UI should be driven by the Prefs state.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Comment on attachment 8902070 [details]
> Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing
> persistence
> 
> https://reviewboard.mozilla.org/r/173482/#review178932
> 
> :::
> devtools/client/webconsole/new-console-output/components/filter-bar.js:172
> (Diff revision 1)
> > +      FilterCheckbox({
> > +        label: l10n.getStr("webconsole.enablePersistentLogs.label"),
> > +        title: l10n.getStr("webconsole.enablePersistentLogs.tooltip"),
> > +        onChange: this.onChangePersistToggle,
> > +        checked: persistLogs,
> >        })
> 
> I thought we wanted to display it in the top bar, not the filter one ?
> If we put it here, maybe we should change the icon (as well as the tooltip)
> of the button to toggle the visibility of this sub-toolbar.
>
> At the moment it says "Toggle the filter bar", which I'm not sure would make
> sense anymore with the persist checkbox in it.
> 
> Maybe we could simply have a cog icon, put it on the right of the
> top-toolbar, and have the title say "Toggle the console settings panel" or
> something alike ?

I don't think we made a decision on that.  Some older screenshots show it in the filter area (https://projects.invisionapp.com/share/GN527IGMZ#/screens/121551039), as does Comment 4, but we may have also discussed putting it by the top bar.  There should be plenty of space to put it there - I will see how that feels.

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8902069 [details]
Bug 1307881 - Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;

https://reviewboard.mozilla.org/r/173480/#review179462

Looks good just one inline comment

Honza

::: devtools/client/locales/en-US/toolbox.dtd:184
(Diff revision 1)
>  <!ENTITY options.screenshot.audio.label      "Play camera shutter sound">
>  <!ENTITY options.screenshot.audio.tooltip    "Enables the camera audio sound when taking screenshot">
>  
> -<!-- LOCALIZATION NOTE (options.commonprefs): This is the label for the heading
> -      of all preferences that affect both the Web Console and the Network
> -      Monitor -->
> +<!-- LOCALIZATION NOTE (options.netmonitor.label): This is the label for the
> +  -  heading of the group of Network Monitor preferences in the options panel. -->
> +<!ENTITY options.netmonitor.label           "Netmonitor">

I would use "Network" instead of "Netmonitor", so the option group title corresponds with the Panel title.

(and we might want to do it for the Console/Web Console option group too)

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, one more thing. The checkbox isn't vertically centered in the Toolbar. You might want to see how "Disable cache" checkbox is done in the Network panel.

Honza

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: Bug 1307881 - Part 3 - Remove unused filteredMessageVisible prop from the state;

Review commit: https://reviewboard.mozilla.org/r/174528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174528/

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8902069 [details]
Bug 1307881 - Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173480/diff/1-2/

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173482/diff/1-2/

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8902845 [details]
Bug 1307881 - Part 3 - Remove unused filteredMessageVisible prop from the state;

https://reviewboard.mozilla.org/r/174528/#review179998

Oops, should have spotted this one when doing the work for hidden messages.
Thanks !

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

https://reviewboard.mozilla.org/r/173482/#review180000

This is looking good to me, and working well.
One minor concern is that it can looks weird when the "hidden messages" label is displayed (https://screenshots.firefox.com/taVUJacCI8FAADvF/null).

::: devtools/client/webconsole/new-console-output/test/components/filter-bar.test.js:246
(Diff revision 2)
> +  it("toggles persist logs when checkbox is clicked", () => {
> +    const store = setupStore([]);
> +
> +    expect(getAllUi(store.getState()).persistLogs).toBe(false);
> +
> +    const wrapper = mount(Provider({store}, FilterBar({ serviceContainer })));
> +    wrapper.find(".filter-checkbox input").simulate("change");
> +
> +    expect(getAllUi(store.getState()).persistLogs).toBe(true);
> +  });

Maybe we could have a test to make sure clicking on the label acts on the checkbox too ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_persist.js:6
(Diff revision 2)
> +// Check that message persistence works - bug 705921 / bug 1307881
> +
> +"use strict";
> +
> +const TEST_URI = "http://example.com/browser/devtools/client/webconsole/new-console-output/test/mochitest/test-console.html";
> +
> +add_task(async function () {

Is this a test based on one that existed before ? If so, could we have it by `hg cp` so we keep the history ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_persist.js:35
(Diff revision 2)
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["devtools.webconsole.persistlog", true]
> +  ]});

Could we click the checkbox instead, and wait for it to be checked with a MutationObserver ?
We can test that the prefs has the expected value after doing that too.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #23)
> Comment on attachment 8902070 [details]
> Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing
> persistence;
> 
> https://reviewboard.mozilla.org/r/173482/#review180000
> 
> This is looking good to me, and working well.
> One minor concern is that it can looks weird when the "hidden messages"
> label is displayed (https://screenshots.firefox.com/taVUJacCI8FAADvF/null).

Good catch - changed some flex properties to fix this.

> :::
> devtools/client/webconsole/new-console-output/test/components/filter-bar.
> test.js:246
> (Diff revision 2)
> > +  it("toggles persist logs when checkbox is clicked", () => {
> > +    const store = setupStore([]);
> > +
> > +    expect(getAllUi(store.getState()).persistLogs).toBe(false);
> > +
> > +    const wrapper = mount(Provider({store}, FilterBar({ serviceContainer })));
> > +    wrapper.find(".filter-checkbox input").simulate("change");
> > +
> > +    expect(getAllUi(store.getState()).persistLogs).toBe(true);
> > +  });
> 
> Maybe we could have a test to make sure clicking on the label acts on the
> checkbox too ?

This doesn't seem to work because the event simulation doesn't know that a click = change

> :::
> devtools/client/webconsole/new-console-output/test/mochitest/
> browser_webconsole_persist.js:6
> (Diff revision 2)
> > +// Check that message persistence works - bug 705921 / bug 1307881
> > +
> > +"use strict";
> > +
> > +const TEST_URI = "http://example.com/browser/devtools/client/webconsole/new-console-output/test/mochitest/test-console.html";
> > +
> > +add_task(async function () {
> 
> Is this a test based on one that existed before ? If so, could we have it by
> `hg cp` so we keep the history ?

There is one, but there are almost no lines in common

> :::
> devtools/client/webconsole/new-console-output/test/mochitest/
> browser_webconsole_persist.js:35
> (Diff revision 2)
> > +  await SpecialPowers.pushPrefEnv({"set": [
> > +    ["devtools.webconsole.persistlog", true]
> > +  ]});
> 
> Could we click the checkbox instead, and wait for it to be checked with a
> MutationObserver ?
> We can test that the prefs has the expected value after doing that too.

Yes, I don't think we even need to add mutation observer - the click should synchronously change the pref

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8902069 [details]
Bug 1307881 - Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173480/diff/2-3/

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173482/diff/2-3/

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8902845 [details]
Bug 1307881 - Part 3 - Remove unused filteredMessageVisible prop from the state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174528/diff/1-2/

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 8902069 [details]
Bug 1307881 - Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173480/diff/3-4/

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8902070 [details]
Bug 1307881 - Part 2 - Provide a UI within the Web Console for managing persistence;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173482/diff/3-4/

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8902845 [details]
Bug 1307881 - Part 3 - Remove unused filteredMessageVisible prop from the state;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174528/diff/2-3/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8bbf4e5c665
Part 1 - Split message persistence prefs into two, one for the netmonitor and one for the console;r=Honza
https://hg.mozilla.org/integration/autoland/rev/27d26ef5a86c
Part 2 - Provide a UI within the Web Console for managing persistence;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a918bec3a72a
Part 3 - Remove unused filteredMessageVisible prop from the state;r=nchevobbe

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a8bbf4e5c665
https://hg.mozilla.org/mozilla-central/rev/27d26ef5a86c
https://hg.mozilla.org/mozilla-central/rev/a918bec3a72a

Comment 34

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Managed to reproduce the initial issue on 57.0a1 (2017-08-31). Investigated the fixes on 57.0a1 (2017-09-05), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6. There are some potential issues there:
- as Honza specified in comment 18, the checkbox isn't vertically centered in the Toolbar (this is mostly visible on Windows);
- when the hidden messages UI is displayed and the console has reduced width, the hidden messages label is overlapping the persistence label and no blur effect is applied (like is done in the main tools tabs case - Inspector, Console, Debugger etc.); this takes place when console has the default position, when is docked on the side of the browser window and when is displayed in a separate window;
  - when the console width is small enough, the hidden messages UI is moved to a second row, but this is not applied on the Mac platform, when the console is docked on the side of the browser window;
- the persist logs checkboxes automatically trigger the corresponding about:config preferences, but the opposite is not applicable: the about:config preferences don't trigger the persist logs checkboxes (the checkboxes state update is made only after the console reopening). I have to mention that after the about:config preferences are triggered, the specific log persistence option is applied, but only the checkbox UI state remains out of date. 
Any thoughts about the above items?

Comment 35

[Johann Hofmann [:johannh]]

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=14eea6bedcf3e2f46ea7c908e1ac9b7d256a42f0&newProject=mozilla-central&newRev=583e73fb8e3c734dbf3a5e13913df7617f5c492c&filter=devtools

You can clearly see the vertical alignment issue mentioned in comment 18 and comment 34 on pretty much all platforms except OSX. It doesn't look pretty. JuliaC, can you file a new bug for that please?

Comment 36

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Johann Hofmann [:johannh] from comment #35)
> Screenshots:
> 
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=14eea6bedcf3e2f46ea7c908e1ac9b7d256a42f0&newProject=mozilla-
> central&newRev=583e73fb8e3c734dbf3a5e13913df7617f5c492c&filter=devtools
> 
> You can clearly see the vertical alignment issue mentioned in comment 18 and
> comment 34 on pretty much all platforms except OSX. It doesn't look pretty.
> JuliaC, can you file a new bug for that please?


Logged bug 1398760.

Comment 37

[Brian Grinstead [:bgrins]]

(In reply to Iulia Cristescu, QA [:JuliaC] from comment #34)
> - when the hidden messages UI is displayed and the console has reduced
> width, the hidden messages label is overlapping the persistence label and no
> blur effect is applied (like is done in the main tools tabs case -
> Inspector, Console, Debugger etc.); this takes place when console has the
> default position, when is docked on the side of the browser window and when
> is displayed in a separate window;
>   - when the console width is small enough, the hidden messages UI is moved
> to a second row, but this is not applied on the Mac platform, when the
> console is docked on the side of the browser window;

Thanks! I believe these two things might be the same issue - it seems there's a certain width where the 'hidden messages' text and reset button have not yet moved down to the second row and the 'persist logs' text gets covered up. On OSX in side docked mode the min width on the toolbox seems to be right at that limit.  Could you please file a new bug for that blocking this one?

> - the persist logs checkboxes automatically trigger the corresponding
> about:config preferences, but the opposite is not applicable: the
> about:config preferences don't trigger the persist logs checkboxes (the
> checkboxes state update is made only after the console reopening). I have to
> mention that after the about:config preferences are triggered, the specific
> log persistence option is applied, but only the checkbox UI state remains
> out of date. 

That's OK, we don't need to support dynamically updating the checkbox from an about:config change right now. We talked about a way to do this in Comment 15 but decided not to do it here.

Comment 38

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Brian Grinstead [:bgrins] from comment #37)
> (In reply to Iulia Cristescu, QA [:JuliaC] from comment #34)
> > - when the hidden messages UI is displayed and the console has reduced
> > width, the hidden messages label is overlapping the persistence label and no
> > blur effect is applied (like is done in the main tools tabs case -
> > Inspector, Console, Debugger etc.); this takes place when console has the
> > default position, when is docked on the side of the browser window and when
> > is displayed in a separate window;
> >   - when the console width is small enough, the hidden messages UI is moved
> > to a second row, but this is not applied on the Mac platform, when the
> > console is docked on the side of the browser window;
> 
> Thanks! I believe these two things might be the same issue - it seems
> there's a certain width where the 'hidden messages' text and reset button
> have not yet moved down to the second row and the 'persist logs' text gets
> covered up. On OSX in side docked mode the min width on the toolbox seems to
> be right at that limit.  Could you please file a new bug for that blocking
> this one?
> 

Thank you Brian! Logged bug 1399125.

Comment 39

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Marking this as verified fixed on Fx 57 build. Bug 1399125 will be assessed separately.