1327731 - Filtering in netmonitor isn't preserved until I close devtools

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1 (good):
0. Open console, dock devtools to the bottom side of the window.
   Set filtering to "CSS" in console, close devtools
1. Open console
2. Set filtering to "JS" instead of "CSS"
3. Open new tab
4. Open console in the tab from Step 3


STR_2 (bad):
0. Open netmonitor, dock devtools to the bottom side of the window.
   Set filtering to "CSS" in netmonitor, close devtools
1. Open netmonitor
2. Set filtering to "JS" instead of "CSS"
3. Open new tab
4. Open netmonitor in the tab from Step 3


AR:
 Console    opened in STR_1 Step 4 has filtering set to "JS"
 Netmonitor opened in STR_2 Step 4 has filtering set to "CSS"

ER:  Either X or Y
 X) Netmonitor opened in STR_2 Step 4 should have filtering set to "JS", i.e. save config immediately.
 Y) Console    opened in STR_2 Step 4 should have filtering set to "CSS", i.e. save config on close.

Note:
 All settings such as height, width, type of filtering should work the same way in devtools.
 If current plan for devtools suggests that user (me) has to {open devtools in a new tab, change configuration, then close devtools in that tab} in order to change config, then it's console bug.
 If current plan is to provide normal UX (save config immediately), then it's bug of all devtools.

Comment 1

[Adrien Enault]

Hello,

I have reproduced the behavior described, and I'd like to try to fix it. As far as I can see, the behavior is that whenever the User changes the filtering on the console, the performance or else, this setting is save and used for the next tab, even if the devtools are not closed.

Should I then align the net monitor to match this behavior ? (I suppose, only for the toggle-able filters, not the free text ones) 

Best regards,

Adrien

Comment 2

[Adrien Enault]

Can you conform and assign me to this bug ?

Comment 3

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

(In reply to Adrien Enault from comment #1)
> Hello,
> 
> I have reproduced the behavior described, and I'd like to try to fix it. As
> far as I can see, the behavior is that whenever the User changes the
> filtering on the console, the performance or else, this setting is save and
> used for the next tab, even if the devtools are not closed.
> 
> Should I then align the net monitor to match this behavior ? (I suppose,
> only for the toggle-able filters, not the free text ones) 
Yes

Assigning to you, thanks for working on this!

Honza

Comment 4

[Adrien Enault]

Attachment: netmonitorFilteringPrefs.patch

Hello,

I've fixed the issue, and now the filtering should be saved immediately. I had to change one of the test to reflect that (and the other to make it work as I have removed a selector that was no longer used). It should already test that the filter is saved immediately in the Prefs.

Best regards,

Adrien

Comment 5

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

Comment on attachment 8835078 [details] [diff] [review]
netmonitorFilteringPrefs.patch

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

Thanks for working on this!

See my inline comment.

Honza

::: devtools/client/netmonitor/reducers/filters.js
@@ +80,5 @@
>          toggleRequestFilterType(state.requestFilterTypes, action));
> +      Prefs.filters = newState.requestFilterTypes.toSeq()
> +        .filter(checked => checked).keySeq().toArray();
> +      return newState;
> +    }

I think that we should not change the preference here. Reducers should be pure functions only changing state and having no side effects.

I would rather set the prefs within the filter actions:
- enableRequestFilterTypeOnly
- toggleRequestFilterType


You can change these actions into thunks, get the state and utilize the getActiveFilters selector:


function enableRequestFilterTypeOnly(filter) {
  return (dispatch, getState) => {
    dispatch({
      type: ENABLE_REQUEST_FILTER_TYPE_ONLY,
      filter,
    });

    const state = getState();
    ... getActiveFilters(state);
    ... set the prefs here
  });
}

We could also introduce new Redux middleware that intercepts these two actions and sets prefs ...

@Ricky: what do you think?

Honza

Comment 6

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

@Ricky: see my previous comment.

Honza

Comment 7

[Adrien Enault]

Attachment: netmonitorFilteringPrefs2.patch

In the meantime, I have updated the patch with your first suggestion.
Br,
Adrien.

Comment 8

[Adrien Enault]

Attachment: netmonitorFilteringPrefs3.patch

And I tried my hand at adding a middleware to save the filters
Br,
Adrien

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835621 [details] [diff] [review]
netmonitorFilteringPrefs3.patch

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

Nice work!

I saw some code quality improvement and it looks great to me.

Yes, the best way to deal with Redux state and any types of storage is middleware.

I think this patch is the right way to go. Thanks!

::: devtools/client/netmonitor/middleware/save-filter-prefs.js
@@ +8,5 @@
> +  ENABLE_REQUEST_FILTER_TYPE_ONLY,
> +  TOGGLE_REQUEST_FILTER_TYPE,
> +} = require("../constants");
> +const { Prefs } = require("../prefs");
> +const { getActiveFilters } = require("devtools/client/netmonitor/selectors/index");

nit: const { getActiveFilters } = require("../selectors/index");

@@ +11,5 @@
> +const { Prefs } = require("../prefs");
> +const { getActiveFilters } = require("devtools/client/netmonitor/selectors/index");
> +
> +function filterPrefs(store) {
> +  return next => {

nit: I'd prefer

return next => action => {
 ...
}

@@ +23,5 @@
> +    }
> +  }
> +}
> +
> +module.exports = filterPrefs;
\ No newline at end of file

nit:

Let's rename save-filter-prefs to prefs.js and module name to Prefs.

There are other features might need to save in prefs in the future as well (ex: width and height of NetworkDetailsPanel). Thus, it would be better to give a more general name.

Comment 10

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

Thanks Ricky!

@Adrien, please let me know before landing yet,
I'd like to look at the final patch

Thanks for working on this!

Honza

Comment 11

[Adrien Enault]

Attachment: netmonitorFilteringPrefs4.patch

Hello,

I have corrected the third patch as suggested by Ricky. I'm not sure how or even if I can land it myself.

Best regards,

Adrien

Comment 12

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

@Adrien: can you please rebase on the latest mc HEAD, I am seeing some failures when applying the patch.

Thanks!

Honza

Comment 13

[Adrien Enault]

Attachment: netmonitorFilteringPrefs5.patch

Hello,

indeed, someone has done some refactor on the netmonitor. Here's the updated diff. I ran the tests of the netmonitor and all passed except two that have nothing to do with the change.

Adrien.

Comment 14

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

Comment on attachment 8836798 [details] [diff] [review]
netmonitorFilteringPrefs5.patch

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

I really like how its done now!

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e983a9dd86ba9f81db1ce1d1d94a7abf31c41180


Please, fix the commit message. Reviewers should be in it.

Bug 1327731 - Filtering in netmonitor is now saved immediately; r=Honza,rickychien


Thanks for the patch!
Honza

Comment 15

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

@Adrien: try push looks good, this thing is ready to land!
(just fix the commit message please)

Honza

Comment 16

[Adrien Enault]

Attachment: netmonitorFilteringPrefs6.patch

Done :D. Updated the commit message

Comment 17

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

Thanks, ready to land.

Honza

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/366dc31565e3
Filtering in netmonitor is now saved immediately. r=Honza, r=rickychien

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/366dc31565e3

Comment 20

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce this bug on Nightly 49.0a1 (2016-05-26) (Build ID: 20160526083057) by the following Comment 0's instruction!

This Bug is now Verified as Fixed on Latest Firefox Nightly 54.0a1 (2017-03-05) (64-bit)

Build ID: 20170305110158
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
OS: Linux 4.8.0-39-generic; Ubuntu 16.04.2 (64 Bit)

Comment 21

[Tanvir Rahman]

I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Windows 10, 64 bit!

The bug's fix is now verified on Latest Beta 54.0b3

Build ID 	20170427091925
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[testday-20170428]