Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[blocked] Call tree filtering #293

Closed

Conversation

gregtatum
Copy link
Member

This is a UI prototype for filtering. This will provide UI for issues #244 and issue #242.

image

image

I'd like to place this UI in a button that opens up, with possibly a text label that goes with it. The placement is arbitrary at this point and probably needs to be adjusted a bit. The button is the same filtering button used throughout all of devtools.

I also tried to find less technical words to describe what was going on, while not sacrificing on the technical accuracy of the filtering operation. The filter panel and the context menu can grow and change with any additional features we wish to add.

I would appreciate any feedback before I started working on the filtering logic part of this.
@bzbarsky @mstange @julienw @ehsan

@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #293 into master will decrease coverage by 0.21%.
The diff coverage is 29.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #293      +/-   ##
==========================================
- Coverage   35.55%   35.33%   -0.22%     
==========================================
  Files         124      126       +2     
  Lines        6542     6763     +221     
  Branches     1415     1466      +51     
==========================================
+ Hits         2326     2390      +64     
- Misses       3645     3783     +138     
- Partials      571      590      +19
Impacted Files Coverage Δ
src/actions/profile-view.js 41.17% <0%> (-7.67%) ⬇️
src/components/calltree/ProfileCallTreeSettings.js 0% <0%> (ø) ⬆️
src/components/calltree/CallTreeFilters.js 0% <0%> (ø)
src/components/shared/ArrowPanel.js 0% <0%> (ø) ⬆️
.../components/calltree/ProfileCallTreeContextMenu.js 0% <0%> (ø) ⬆️
src/profile-logic/profile-data.js 66.45% <100%> (+1.75%) ⬆️
src/test/utils/analyze-profile.js 100% <100%> (ø)
src/url-handling.js 32% <20%> (-1.34%) ⬇️
src/reducers/url-state.js 73.52% <25%> (-10.4%) ⬇️
src/reducers/profile-view.js 60.57% <53.84%> (-0.33%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a6887d...9e68701. Read the comment docs.

@bzbarsky
Copy link

bzbarsky commented May 3, 2017

The main problem I see is that "Hide" could mean two things. It could mean "Charge to caller" or it could mean "Exclude from profile" (so like the existing "only show samples where the stack contains X" filter, but only showing samples where the stack does not contain X). I'd actually want both of those to be available...

Maybe those could be labeled "Treat as part of caller" and "Exclude" respectively, if we want less-technical labels?

@gregtatum gregtatum changed the title [WIP] Prototype UI for call tree filtering such as charge to caller. Prototype UI for call tree filtering such as charge to caller. May 19, 2017
@gregtatum gregtatum requested a review from mstange May 19, 2017 20:39
@gregtatum
Copy link
Member Author

@mstange This should be ready for review, sorry in advance for the large PR.

@gregtatum gregtatum changed the title Prototype UI for call tree filtering such as charge to caller. Call tree filtering May 23, 2017
@hotsphink
Copy link
Contributor

"Drop" / "Include in caller time" ? Or maybe "Merge into caller"? "Merge with caller"? "Fold into caller"?

At some point, I'd like the context menu to include 'Category "X"' along with 'Library "Y"'.

@gregtatum
Copy link
Member Author

@mstange do you think you could find some time to look at this? I would like to get it merged in.

@mstange
Copy link
Contributor

mstange commented Jun 7, 2017

I found a few things during testing:

  • The time of the pruned function is charged to the caller. This surprised me. I'd prefer to use a different name for this operation; all of Steve's suggestion sound like better alternatives to me.
  • If the window is not very high, adding many call tree filters will cause the outer page to become scrollable, even if the filter list is not visible. (It looks like that's because it's only opacity:0 but not display:none.)
  • If you hide a function, and then focus on a call stack within that hidden function, you get an empty call tree.
  • The animation looks funny but didn't actually help me understand that the filter goes into the filter icon. How about this animation instead: When adding a filter, fade in the function name next to the fillter icon, wait a bit, and then make it move to the left, passing through the filter icon but clipped in such a way that it's not visible on the left side of the icon. At the same time, make the scale of the filter icon bounce.

Copy link
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found anything to complain about in the code, except for the naming issues. I'll leave it up to you to decide whether to fix the bugs before merging or after.

thread: Thread,
filter: IndexIntoFuncTable => boolean
filterOutByFunc: IndexIntoFuncTable => boolean,
pruneByFunc: IndexIntoFuncTable => boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be called filterOutFrameByFunc and filterOutSubtreeByFunc or something along those lines.

@mstange
Copy link
Contributor

mstange commented Jun 8, 2017

Here's my inspiration for the animation I suggested: http://phlsa.github.io/ff-bubble-animation/
If you click the UX button in the address bar, and then click somewhere else to dismiss the panel, you can see the UX button grow briefly.

@gregtatum gregtatum changed the title Call tree filtering [blocked] Call tree filtering Jul 25, 2017
@gregtatum
Copy link
Member Author

Overcome through a separate implementation.

@gregtatum gregtatum closed this Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants