Improve dropped functions UI #4682

mstange · 2023-06-22T19:51:51Z

┆Issue is synchronized with this Jira Task

This PR implements some changes to the arrow panel code which make arrow panels usable for the dropped functions panel in the call tree settings. With the previous implementation, I encountered these issues: - When the source view was open, the panel would be clipped to the call tree bounds and could not overlap the source view. The overflow:auto clip from the splitter view was applying to the panel. - With a narrow viewport or an unusually positioned anchor element, the panel would sometimes be positioned partially offscreen. - It was tricky to enforce a maximum height on the panel in a way that squished the list box inside the panel. - The panel would reposition itself based on the size of the anchor element *while the panel was open*. E.g. you would click an "x" button to remove a dropped function, and the anchor button's label would change from "10 dropped functions" to "9 dropped functions", causing the button to shrink. Then the panel would shift slightly to the left, moving the "x" button out from under the mouse cursor. This commit fixes all of these issues. The most important change is the switch to position:fixed: Now the panel will no longer be clipped by overflow:hidden/auto ancestor of the button that opens the panel. Furthermore, the panel position is only computed during opening and then remains unchanged while the panel is open. And there is some logic to make the panel fit onscreen (while also making sure that the arrow still points at the right place).

This has a few advantages: - You'll be able to remove dropped functions after you've pushed other transforms on the transform stack - The transform stack doesn't become cluttered when you drop many functions - When loading a profile with many dropped functions, computing the filtered thread is much faster because we don't compute a new samples table for each individual dropped function - External tools which use the web console API (by dispatching actions manually) can drop multiple functions with a single dispatch, greatly reducing the time spent in per-dispatch renders

codecov · 2023-07-05T20:57:00Z

Codecov Report

Patch coverage: 69.67% and project coverage change: -0.13 ⚠️

Comparison is base (eac238a) 88.33% compared to head (f3001f2) 88.21%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4682      +/-   ##
==========================================
- Coverage   88.33%   88.21%   -0.13%     
==========================================
  Files         299      301       +2     
  Lines       26709    26829     +120     
  Branches     7206     7231      +25     
==========================================
+ Hits        23594    23667      +73     
- Misses       2902     2944      +42     
- Partials      213      218       +5

Impacted Files	Coverage Δ
src/components/app/MenuButtons/Permalink.js	`81.81% <ø> (ø)`
src/components/app/MenuButtons/index.js	`88.17% <ø> (ø)`
src/components/shared/StackSettings.js	`86.36% <ø> (ø)`
src/profile-logic/profile-data.js	`93.51% <0.00%> (-0.30%)`	⬇️
src/utils/flow.js	`84.28% <ø> (-0.23%)`	⬇️
src/components/shared/DroppedFunctionsPanel.js	`5.88% <5.88%> (ø)`
src/selectors/per-thread/thread.js	`91.94% <50.00%> (-3.06%)`	⬇️
src/actions/profile-view.js	`90.13% <57.14%> (-0.37%)`	⬇️
src/reducers/url-state.js	`95.59% <61.29%> (-2.66%)`	⬇️
src/app-logic/url-handling.js	`86.34% <77.27%> (-0.40%)`	⬇️
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mstange · 2023-07-05T21:25:21Z

This is ready for review now. It's not super polished visually, but I think it's a vast improvement over what we have today. Also, I really want the dropFunctions action for the report generator.

It's also missing some tests.

flodolo · 2023-07-06T05:06:45Z

locales/en-US/app.ftl

@@ -993,6 +987,53 @@ TransformNavigator--collapse-function-subtree = Collapse subtree: { $item }
 #   $item (String) - Search filter of the markers that transform will apply to.
 TransformNavigator--drop-samples-outside-of-markers-matching = Drop samples outside of markers matching: “{ $item }”

+## "Dropped functions" settings UI
+##
+## A "dropped function" is a function which is excluded from the sample data.


A comment this long is not helpful for localization, it's going to clutter the UI given its size. Comments on the individual strings are OK.

flodolo · 2023-07-06T05:07:19Z

locales/en-US/app.ftl

+    .label =
+        { $numberOfDroppedFunctions ->
+            [one] 1 dropped function
+        *[other] { $numberOfDroppedFunctions } dropped functions


nit: fix the indentation. I would also shorten the variable name, e.g. just $num.

julienw

Thanks, this looks pretty good to me. I left a few comments (on the 2nd commit only), tell me what you think!

julienw · 2023-07-06T13:40:45Z

src/actions/profile-view.js

+  return (dispatch) => {
+    dispatch({
+      type: 'DROP_FUNCTIONS',
+      threadsKey,
+      functionIndexes,
+    });
+  };


you don't need a thunk action, you can return the action directly.
(same below)

julienw · 2023-07-06T13:43:14Z

src/app-logic/url-handling.js

@@ -407,6 +408,14 @@ export function getQueryStringFromUrlState(urlState: UrlState): string {
            urlState.profileSpecific.transforms[selectedThreadsKey]
          ) || undefined;
      }
+      if (
+        selectedThreadsKey !== null &&
+        urlState.profileSpecific.droppedFunctions[selectedThreadsKey]


you should probably check whether it's an empty array?

julienw · 2023-07-06T13:57:58Z

src/types/transforms.js

-  /**
-   * The DropFunction transform removes samples from the thread that have a function
-   * somewhere in its stack.
-   *
-   *                 A:4,0                              A:1,0
-   *                 /    \        Drop Func C            |
-   *                v      v           -->                v
-   *            B:3,0     C:1,0                         B:1,0
-   *           /      \                                   |
-   *          v        v                                  v
-   *        C:2,1     D:1,1                             D:1,1
-   *        |
-   *        v
-   *      D:1,1
-   */


I wonder if we could put this ascii art schema somewhere?

julienw · 2023-07-06T14:00:16Z

src/components/shared/DroppedFunctionsSetting.js

+type DispatchProps = {|
+  +undropFunctions: typeof undropFunctions,
+|};


This doesn't seem to be used here after all.

julienw · 2023-07-06T14:01:34Z

src/components/shared/DroppedFunctionsSetting.js

+
+  render() {
+    const { droppedFunctions } = this.props;
+    const numberOfDroppedFunctions = droppedFunctions.length;


optional nit: I'd take the length as a prop (and compute it in mapStateToProps) instead of the full array. Indeed the component only needs this information.

julienw · 2023-07-06T15:43:41Z

src/components/shared/DroppedFunctionsPanel.css

+  padding: 1px;
+  border: 0;
+  border-radius: 2px;
+  background: url(../../../res/img/svg/close-dark.svg) no-repeat center;


nit:
you can use firefox-profiler-res instead of these ../../. It's not shorter but easier to use.

julienw · 2023-07-06T15:47:14Z

src/components/shared/DroppedFunctionsPanel.js

+  DispatchProps
+>({
+  mapStateToProps: (state) => ({
+    droppedFunctions: selectedThreadSelectors.getDroppedFunctions(state),


I think you don't need this prop, you can do everything with just droppedFunctionsDisplayData.

julienw · 2023-07-06T15:50:00Z

src/components/shared/StackSettings.js

@@ -88,6 +89,9 @@ class StackSettingsImpl extends PureComponent<Props> {
              <StackImplementationSetting />
            </li>
          ) : null}
+          <li className="panelSettingsListItem">
+            <DroppedFunctionsSetting />


I wonder if it should be present when no function are dropped yet... What do you think?

julienw · 2023-07-06T15:51:51Z

locales/en-US/app.ftl

+# This message is shown inside the list of dropped functions when the list is empty.
+# It instructs the user about the purpose of the list.
+# The text "Drop samples with this function" should be consistent with the
+# translation of `CallNodeContextMenu--transform-drop-function`.


@flodolo do you think it would be good to reuse directly the value for that translation here?
Although we might not need it anymore if we don't display it when there's no function like I suggest elsewhere.

Yes, this is pretty much the only use case where using a message reference makes sense.

The only downside is that, if the referenced message is not translated yet, we'll show the referenced ID (and not fall back to the English text).

julienw · 2023-07-06T16:01:14Z

src/components/shared/DroppedFunctionsSetting.js

+type Props = ConnectedProps<{||}, StateProps, DispatchProps>;
+
+class DroppedFunctionsSettingImpl extends PureComponent<Props> {
+  _panelPosition = { anchorEdge: 'left', distanceFromEdge: 200 };


This magic number 200 feels weird, maybe we should be explicit and specify the panelWidth: 400 here as well even if it's the default.

Random thought: maybe we'd need to support percentages instead of absolute lengths? Maybe also it's possible to support them by using calc instead of computing in JS.. not sure though...

mstange self-assigned this Jun 22, 2023

mstange mentioned this pull request Jun 28, 2023

Reserve functions for collapsed resources ahead-of-time. #4687

Merged

mstange added 2 commits July 5, 2023 12:44

mstange force-pushed the dropped-functions branch from a30cbff to f3001f2 Compare July 5, 2023 20:49

mstange marked this pull request as ready for review July 5, 2023 20:51

mstange requested a review from a team as a code owner July 5, 2023 20:51

mstange requested a review from julienw July 5, 2023 20:52

flodolo reviewed Jul 6, 2023

View reviewed changes

julienw reviewed Jul 6, 2023

View reviewed changes

mstange marked this pull request as draft June 27, 2024 17:30

mstange mentioned this pull request Jun 27, 2024

When lots of functions are dropped, they're not easy to manage in the UI #4678

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dropped functions UI #4682

Improve dropped functions UI #4682

mstange commented Jun 22, 2023 •

edited by data-sync-user

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

mstange commented Jul 5, 2023

flodolo Jul 6, 2023

flodolo Jul 6, 2023

julienw left a comment

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

julienw Jul 6, 2023

flodolo Jul 6, 2023

julienw Jul 6, 2023

Improve dropped functions UI #4682

Are you sure you want to change the base?

Improve dropped functions UI #4682

Conversation

mstange commented Jun 22, 2023 • edited by data-sync-user Loading

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

mstange commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstange commented Jun 22, 2023 •

edited by data-sync-user

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading