Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Filtering in netmonitor isn't preserved until I close devtools

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Developer Tools: Netmonitor
P3
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: arni2033, Assigned: Adrien Enault)

Tracking

({good-first-bug})

Trunk
Firefox 54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 months ago
>>>   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.
(Reporter)

Updated

7 months ago
No longer blocks: 1277113
(Reporter)

Updated

7 months ago
Component: Untriaged → Developer Tools
Component: Developer Tools → Developer Tools: Netmonitor
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 1

6 months ago
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
(Assignee)

Comment 2

6 months ago
Can you conform and assign me to this bug ?
Flags: needinfo?(odvarko)
(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
Assignee: nobody → schwartzmorn+bugzilla
Flags: needinfo?(odvarko)
(Assignee)

Comment 4

5 months ago
Created attachment 8835078 [details] [diff] [review]
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
Attachment #8835078 - Flags: review?(odvarko)
Status: NEW → ASSIGNED
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
Attachment #8835078 - Flags: review?(odvarko) → review-
@Ricky: see my previous comment.

Honza
Flags: needinfo?(rchien)
(Assignee)

Comment 7

5 months ago
Created attachment 8835597 [details] [diff] [review]
netmonitorFilteringPrefs2.patch

In the meantime, I have updated the patch with your first suggestion.
Br,
Adrien.
Attachment #8835078 - Attachment is obsolete: true
(Assignee)

Comment 8

5 months ago
Created attachment 8835621 [details] [diff] [review]
netmonitorFilteringPrefs3.patch

And I tried my hand at adding a middleware to save the filters
Br,
Adrien
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.
Attachment #8835621 - Flags: review+
Flags: needinfo?(rchien)
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
(Assignee)

Comment 11

5 months ago
Created attachment 8836506 [details] [diff] [review]
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
Attachment #8835597 - Attachment is obsolete: true
Attachment #8835621 - Attachment is obsolete: true
Flags: needinfo?(odvarko)
@Adrien: can you please rebase on the latest mc HEAD, I am seeing some failures when applying the patch.

Thanks!

Honza
Flags: needinfo?(odvarko) → needinfo?(schwartzmorn+bugzilla)
(Assignee)

Comment 13

5 months ago
Created attachment 8836798 [details] [diff] [review]
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.
Attachment #8836506 - Attachment is obsolete: true
Flags: needinfo?(schwartzmorn+bugzilla) → needinfo?(odvarko)
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
Attachment #8836798 - Flags: review+
@Adrien: try push looks good, this thing is ready to land!
(just fix the commit message please)

Honza
Flags: needinfo?(odvarko) → needinfo?(schwartzmorn+bugzilla)
(Assignee)

Comment 16

5 months ago
Created attachment 8837287 [details] [diff] [review]
netmonitorFilteringPrefs6.patch

Done :D. Updated the commit message
Attachment #8836798 - Attachment is obsolete: true
Flags: needinfo?(schwartzmorn+bugzilla) → needinfo?(odvarko)
Attachment #8837287 - Flags: review+
Thanks, ready to land.

Honza
Flags: needinfo?(odvarko)
Keywords: checkin-needed

Comment 18

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/366dc31565e3
Filtering in netmonitor is now saved immediately. r=Honza, r=rickychien
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/366dc31565e3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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)
QA Whiteboard: [testday-20170303]

Comment 21

3 months ago
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]
You need to log in before you can comment on or make changes to this bug.