Implement marker filter menu and "drop samples outside of marker filter" transform#4631
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4631 +/- ##
==========================================
- Coverage 88.61% 88.49% -0.13%
==========================================
Files 294 295 +1
Lines 26095 26196 +101
Branches 7035 7062 +27
==========================================
+ Hits 23125 23182 +57
- Misses 2765 2805 +40
- Partials 205 209 +4
☔ View full report in Codecov by Sentry. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
This looks pretty good to me! I don't have much to say :-) Thanks for working on this!
| const { isFilterMenuVisibleOnMouseDown } = this.state; | ||
| if (isFilterMenuVisibleOnMouseDown) { | ||
| // Do nothing as we would like to hide the menu if the menu was already visible on mouse down. | ||
| return; | ||
| } |
There was a problem hiding this comment.
aaaah so that's what we should do for the track menu as well 😁
I wonder if we could be able to extract this for an easier reuse...
(not for now)
There was a problem hiding this comment.
Yeah, I was thinking about the track menu as well while doing this :) We can possibly extract it.
|
Could you add a dropdown arrow to the icon? Also, I have the same problem with "filtered markers" as last time - does it mean "markers which match the filter" or "markers which have been filtered out"? How about "markers matching <...>"? |
|
Thanks for the feedback! I added 2 new commits to add an arrow icon and make the wording more explicit. Please let me know what you think! |
looks good to me |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Some more comments to simplify this patch further. I hope this will be ok!
| type StateProps = {| | ||
| +searchString: string, | ||
| +threadsKey: ThreadsKey, | ||
| +isMarkerFiltersMenuVisible: boolean, |
There was a problem hiding this comment.
This isn't used in this component.
There was a problem hiding this comment.
Oh yeah, I think I added it while experimenting and then forgot. Removed it.
| _onShow = () => { | ||
| this.props.setMarkerFiltersMenuVisibility(true); | ||
| }; | ||
|
|
||
| _onHide = () => { | ||
| this.props.setMarkerFiltersMenuVisibility(false); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I've been thinking some more during the night (😴), I think it should be possible to just pass it on to the parent (add onShow / onHide props to MarkerFiltersContextMenuImpl, keep a local state in MarkerSettings) instead of using the redux state. We don't use this state elsewhere so it's better to keep it local IMO.
What do you think?
There was a problem hiding this comment.
It works for me too. I initially wanted to keep that in the redux state thinking that we might need to change it in other places too, but this sounds simpler. I changed the code to keep that in local state.
…ransforms
Before the 'filter-samples' transform, we didn't need to get the
derived markers to be able to compute any thread related pre-thread
selectors. But with 'filter-samples' transform, we need to get the
derived markers to be able to compute the transform filtered thread
because this transform needs to find the markers that match the filter
and get the interval range out of them.
So because of this, I had to split the thread related selectors so we
can get the marker selectors in between them and have the marker
selectors in the second phase of thread selectors.
Previously the dependency graph was like this:
```
thread selectors
|
v
stack and sample
+
marker selectors
|
v
composed selectors
```
Now it's like this:
```
basic thread selectors
|
v
marker selectors
|
v
rest of the thread selectors
|
v
stack and sample selectors
|
v
composed selectors
```
We can't put these thread selectors that require markers into the
`composed` selectors because stack and sample selectors require those
selectors earlier in the pipeline and I wanted to keep the thread
selectors still in their file to keep the pipeline in a single file.
4 participants
This is the successor of #4610, fixes #4590.
Deploy preview
After getting feedback from performance and sp3 people, we decided to implement this feature in a different way. I'm going to close the other PR in favor of this one.
Differently in this PR, we have a new "marker filters" menu on the right side of the search filter. When you click on that, a new context menu will appear and you will see the "Drop samples outside of filtered markers" context menu item.
When this is applied, you will be able to remove the samples that are outside of the filtered markers.
Here's the new "marker filter" button:
When you click on this button, this panel will pop up:
Implementation details:
First commit is unchanged, second commit is heavily changed but still similar to the initial version. And the rest is pretty much changed all together.