1440068 - Filtering messages does not work on groups

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Greg Tatum [:gregtatum]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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)?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

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 3

[Abhinav Koppula]

Attachment: Bug 1440068 - Filtering messages in webconsole on groups.

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 5

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

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

[Patrick Brosset <:pbro>]

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1440068 - Fix filtering on console groups.

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 12

[Miguel Roncancio]

I would like to work on this!

Comment 13

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

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

Comment 14

[Miguel Roncancio]

Attachment: Bug 1440068 - Fix filtering on console groups

Comment 15

[Miguel Roncancio]

Hello! I've submitted a patch that fixes the bug and passes the tests that Nicolas added.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79707fcdaa2d
Fix filtering on console groups r=nchevobbe

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/79707fcdaa2d