Filtering messages does not work on groups
Categories
(DevTools :: Console, defect, P3)
Tracking
(firefox83 fixed)
Tracking | Status | |
---|---|---|
firefox83 | --- | fixed |
People
(Reporter: gregtatum, Assigned: mroncancio19, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
Group messages are not filterable. Various STR: * From the console run: console.log("aaa"); console.log("bbb"); console.group("ccc"); console.log("aaa"); console.groupEnd("ccc"); This gives: > aaa > bbb > v ccc > | aaa * Enter the filter: bbb ER: > bbb AR: > bbb > v ccc
Reporter | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
These are currently skipped from filtering explicitly: https://dxr.mozilla.org/mozilla-central/rev/861067332bac96a44bbf41ef366f58a30476057b/devtools/client/webconsole/new-console-output/reducers/messages.js#534. I don't see why they shouldn't be filtered - although some care will be required to make sure they do show up if they have a child message that matches (while still filtering out non-matching children). Also, what if the group header text matches - should we then include the entire group (including children)?
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 2•6 years ago
|
||
I agree that if a text search match a group, we should not filter its "children". So given the following example: > ▼ aaa > | bbb > | ccc > | ▼ ddd > | | eee > fff > gggg Here's what we should have with : - filter text "aa": > ▼ aaa > | bbb > | ccc > | ▼ ddd > | | eee - filter text "bb": > ▼ aaa > | bbb - filter text "dd": > ▼ aaa > | ▼ ddd > | | eee - filter text "ee": > ▼ aaa > | ▼ ddd > | | eee - filter text "gg": > gggg
Comment hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8975376 [details] [diff] [review] Bug 1440068 - Filtering messages in webconsole on groups. https://reviewboard.mozilla.org/r/243670/#review249544 Hello abhinav, thanks a lot for the patch ! I have some questions about it, and also, we should have tests for the different cases (a matching group should have its children displayed, a matching child should have all its parents displayed, a matching group should have both its parents displayed and all its children, …) ::: devtools/client/webconsole/reducers/messages.js:355 (Diff revision 1) > + let parentMsgIds = []; > + messagesToShow.forEach(msgId => { > + let groupIds = []; > + let message = messagesById.get(msgId); > + // push the immediate parent > + groupIds.push(message.groupId); maybe we should continue when message.groupId is falsy, so we don't have null values in the groupIds array ? ```js if (!message.groupId) { continue; } ``` ::: devtools/client/webconsole/reducers/messages.js:361 (Diff revision 1) > + groupIds.forEach(groupId => { > + if (!parentMsgIds.includes(groupId)) { > + parentMsgIds.push(groupId); > + } > + }); instead of doing this, maybe we could have parentMsgIds as a Set ? ::: devtools/client/webconsole/reducers/messages.js:368 (Diff revision 1) > + // if msg is a group then add all its children > + let childIds = []; > + messagesById.forEach((message, msgId) => { > + if (messagesToShow.includes(message.groupId) || childIds.includes(message.groupId)) { > + childIds.push(msgId); > + } > + }); I feel like this should be done in the firs loop (starting l.350), so we don't iterate twice over the list ::: devtools/client/webconsole/reducers/messages.js:376 (Diff revision 1) > + let finalMsgsToShow = []; > + messagesById.forEach((message, msgId) => { > + if (childIds.includes(msgId) || // if message is a group, all children of that group should be shown > + messagesToShow.includes(msgId) || // the individual filtered message > + parentMsgIds.includes(msgId)) { // if message is being shown, then all ancestors should be shown > + if (!finalMsgsToShow.includes(msgId)) { > + finalMsgsToShow.push(msgId) > + } > + } > + }); i am not sure about using intermediates structures here. In the first loop, we could directly append messages id to `messagesToShow`. I feel like this would make the code a bit easier to follow, what do you think ? ::: devtools/client/webconsole/reducers/messages.js (Diff revision 1) > - // Some messages can't be filtered out (e.g. groups). > - // So, always return visible: true for those. > - if (isUnfilterable(message)) { > - return { > - visible: true > - }; > - } why remove this ? if I remember correctly, we still don't want evaluation results to be filtered out
Updated•6 years ago
|
Comment 5•6 years ago
|
||
Moving to p3 because no activity for at least 24 weeks. See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Comment 6•5 years ago
|
||
Hi Abhinav,
I'm going to unassign you from the bug for now, since it hasn't been touched in almost 1 year.
If you did still intend to work on it, please feel free to claim it again.
Thanks
Updated•4 years ago
|
Comment 8•4 years ago
|
||
Comment 9•4 years ago
|
||
If someone wants to work on this, I pushed a patch which only removes the part where we say the console group messages shouldn't be filtered: https://phabricator.services.mozilla.com/D70167
It also adds a test that assert how filtering should work in regards to groups. The test does fail at the moment, and the goal of this bug would be to make it pass
Assignee | ||
Comment 12•4 years ago
|
||
I would like to work on this!
Comment 13•4 years ago
|
||
Thanks for the help Miguel, assigned to you.
Note that Nicolas is on PTO and so getting review for your patch might take some time. He'll be back in 2 weeks.
Honza
Assignee | ||
Comment 14•4 years ago
|
||
Assignee | ||
Comment 15•4 years ago
|
||
Hello! I've submitted a patch that fixes the bug and passes the tests that Nicolas added.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 16•4 years ago
|
||
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79707fcdaa2d Fix filtering on console groups r=nchevobbe
Comment 17•4 years ago
|
||
bugherder |
Description
•