Add a search filter to the timeline track context menu #3634

canova · 2021-11-02T18:07:30Z

This PR adds a search filter to the track context menu. And also changes the "show all tracks" button to "show all tracks below" when a search text is entered. This should help on showing a subset of tracks more easily.

The new search filter:

Some example profiles to test it with:
Profile 1
Profile 2
Profile 3

codecov · 2021-11-02T18:15:26Z

Codecov Report

Merging #3634 (8a759e5) into main (7b4ea84) will decrease coverage by 0.13%.
The diff coverage is 76.21%.

@@            Coverage Diff             @@
##             main    #3634      +/-   ##
==========================================
- Coverage   88.91%   88.77%   -0.14%     
==========================================
  Files         258      259       +1     
  Lines       21545    21746     +201     
  Branches     5507     5564      +57     
==========================================
+ Hits        19156    19305     +149     
- Misses       2214     2262      +48     
- Partials      175      179       +4

Impacted Files	Coverage Δ
src/selectors/url-state.js	`98.48% <ø> (+0.53%)`	⬆️
src/components/shared/TrackSearchField.js	`53.33% <53.33%> (ø)`
src/profile-logic/tracks.js	`85.90% <62.85%> (-7.23%)`	⬇️
src/components/timeline/TrackContextMenu.js	`91.22% <91.89%> (+0.19%)`	⬆️
src/reducers/url-state.js	`97.87% <92.85%> (-0.23%)`	⬇️
src/utils/string.js	`96.87% <92.85%> (-3.13%)`	⬇️
src/actions/profile-view.js	`85.39% <100.00%> (+0.12%)`	⬆️
src/reducers/app.js	`99.32% <100.00%> (+<0.01%)`	⬆️

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 7b4ea84...8a759e5. Read the comment docs.

flodolo

(for the FTL bits)

canova · 2021-11-04T11:54:11Z

@julienw I've added two more commits now and I believe this is ready for review. Could you take a look at it?

Also, about the last commit(0809704): I'm didn't like my solution that much, but I couldn't find another way and it looks like there are some codes that does the same (like in FlameGraph). I had to go in this direction instead of autofocus property because it doesn't work with this prop since this component is already rendered and hidden at the page load. When you click on the context menu button, it simply steals the focus and makes the autofocus useless.

julienw

Praise: this is all working pretty well!

I have quite a strong concern about storing this data to Redux and persisting it in the URL, and that's why I'm marking the patch with "request changes". But the rest of the code looks great!

julienw · 2021-11-08T15:59:54Z

src/components/shared/IdleSearchField.js

+  focus = () => {
+    // Allow time for React to let the event bubble up. Otherwise clicked button
+    // will steal the focus.
+    requestAnimationFrame(() => {


have you tried using a simple setTimeout ?

Also I believe this rAF/setTimeout should rather be in TrackContextMenu's _onShow function than here.

I don't think there is a difference in terms of the result of both setTimeout or rAF. And there are other use cases in our codebase with rAF, so I wanted to use it. For example:

profiler/src/components/app/KeyboardShortcut.js

Lines 77 to 82 in 9f1149a

requestAnimationFrame(() => {

// Restore focus, but not during a React setState call, otherwise this triggers

// a React warning:

// "unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering."

focusAfterClosed.focus();

});

yeah, Greg is a big fan of rAF ;-)

My main concern with rAF is that they're tied with the graphics ticks, and so I'm not so sure this is the right tool for the job for focus calls. Here the only thing we want is to be asynchronous, that's why setTimeout seems to be a better tool.

I'm not sure this does a real difference in this specific case. Though it's important to understand fully the behaviors and interactions for all these functions with callbacks.

And TBH I'm not so sure myself, so I might even be wrong here!

julienw · 2021-11-08T16:02:29Z

src/actions/profile-view.js

@@ -1214,6 +1214,16 @@ export function changeNetworkSearchString(searchString: string): Action {
  };
 }

+/**


(This is a comment for the complete 2nd commit about adding a redux state)
I'm not convinced that this information should go to the redux state and to the URL. This is confusing because this input is local to the menu. Also I wouldn't want the context menu to be automatically filtered when I load a profiler URL.

I'd agree for an input that would control the display of the tracks in the timeline, but here that's not the case, it's really local to this menu and IMO this should stay in a local state in the menu. So I'd move everything from the reducer and the action and the selector back to the TrackContextMenu component. Most of the selector code is in tracks so we just need to call it from here instead of from the selector. This won't be memoized then but I think it's fine (and if we need memoization to happen for perf reason we could still do it explicitely).

BTW I also believe it should be empty each time we open the menu.

What do you think?

(BTW the current code has a "nice" bug because of this: 1. add some filtering terms, 2. right click on a track name in the timeline => tracks are still filtered out).

Also if it's not in Redux anymore, I'd love to see if we need the IdleSearchInput, or whether a plain normal input wouldn't work. Of course the IdleSearchInput brings some nice things like the clear button, so I'm not opposed to keeping it, but maybe lowering the debouncing delay could be nice.

julienw · 2021-11-08T16:28:31Z

src/components/shared/TrackSearch.css

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+.trackSearchField form.idleSearchField {
+  width: 100%;


Is this width necessary? I'm not so sure.

Yes, it is needed because otherwise it doesn't contain 100% of the width.

src/components/shared/TrackSearch.css

src/components/shared/TrackSearch.js

julienw · 2021-11-08T16:58:44Z

src/components/timeline/TrackContextMenu.js

+    showProvidedTracks(
+      searchFilteredGlobalTracks,
+      searchFilteredLocalTracksByPid
+    );


I think I'd prefer most (if not all) of the code in the action showProvidedTracks to be moved here. In the past we've put too much code in thunk actions and I'd like, if possible, that we try to avoid it. In this case I believe we have all the necessary objects in this component already.

Yeah, I guess it's better to move the logic inside the component instead of the thunk action. I mostly did it to follow the current style, but don't mind moving it. Did it.

src/test/components/TrackContextMenu.test.js

src/components/timeline/TrackContextMenu.js

src/profile-logic/tracks.js

canova

Thanks for the review Julien. It took me some time to come back to this PR. I've addressed your review comments. Removed the redux state and made the search filter a state instead. I also had to merge some of the commits to make the huge changes work. Hope it's not going to be make the review harder.

canova · 2021-11-17T21:56:55Z

src/components/shared/TrackSearch.css

+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+.trackSearchField form.idleSearchField {
+  width: 100%;


Yes, it is needed because otherwise it doesn't contain 100% of the width.

canova · 2021-11-17T22:22:49Z

src/components/timeline/TrackContextMenu.js

+    showProvidedTracks(
+      searchFilteredGlobalTracks,
+      searchFilteredLocalTracksByPid
+    );


Yeah, I guess it's better to move the logic inside the component instead of the thunk action. I mostly did it to follow the current style, but don't mind moving it. Did it.

src/components/timeline/TrackContextMenu.js

canova · 2021-11-17T22:47:59Z

src/components/shared/IdleSearchField.js

+  focus = () => {
+    // Allow time for React to let the event bubble up. Otherwise clicked button
+    // will steal the focus.
+    requestAnimationFrame(() => {


I don't think there is a difference in terms of the result of both setTimeout or rAF. And there are other use cases in our codebase with rAF, so I wanted to use it. For example:

profiler/src/components/app/KeyboardShortcut.js

Lines 77 to 82 in 9f1149a

requestAnimationFrame(() => {

// Restore focus, but not during a React setState call, otherwise this triggers

// a React warning:

// "unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering."

focusAfterClosed.focus();

});

src/profile-logic/tracks.js

julienw

Thanks for this work, I think this will be a wonderful addition!

I'm approving now because that's mostly nits and comment changes. If you do the change to prevent "enter" from closing the menu, I'd like to see the code again though :-)

Also if you want to ask a review again after the changes, I can also have another look! In that case please do not change the existing commits, and instead add all changes in 1 or more separate commits :-)

Thanks again

src/app-logic/url-handling.js

src/test/components/TrackContextMenu.test.js

src/components/shared/TrackSearchField.css

src/components/timeline/TrackContextMenu.js

This is both better in terms of merging different paths into one, and also it will be needed in the next patch.

…hen there is a search filter

…onent

…on and remove the identical ones from the children

canova requested a review from a team as a code owner November 2, 2021 18:07

canova force-pushed the track-search branch from c6f74dd to b723565 Compare November 2, 2021 18:15

flodolo approved these changes Nov 2, 2021

View reviewed changes

canova force-pushed the track-search branch from b723565 to 0809704 Compare November 4, 2021 11:50

canova requested a review from julienw November 4, 2021 11:50

julienw requested changes Nov 8, 2021

View reviewed changes

canova force-pushed the track-search branch 3 times, most recently from cf46f3d to f13f3cb Compare November 18, 2021 15:13

canova commented Nov 18, 2021

View reviewed changes

canova requested a review from julienw November 18, 2021 15:15

canova force-pushed the track-search branch from f13f3cb to 3462260 Compare November 18, 2021 16:23

julienw approved these changes Nov 19, 2021

View reviewed changes

canova added 10 commits November 22, 2021 16:27

Move the local tracks rendering inside the global track rendering

91bd577

This is both better in terms of merging different paths into one, and also it will be needed in the next patch.

Move stringsToRegExp function to app logic

a7a6906

Add a search filter to the timeline track context menu

8eb1895

Change the "show all tracks" button to show "show all tracks below" w…

e92e6e5

…hen there is a search filter

Add some tests for track filtering in the track context menu

24cdd2f

Add some tests for the "show all tracks below" button

1eea4dc

Move the logic inside the showProvidedTracks thunk action to the comp…

d66f232

…onent

Address some review comments

51a1893

Move splitSearchString and stringsToRegExp to src/utils/string file

61aca28

Move the changeSearchFilter and fakeTimers to the parent setup functi…

8a759e5

…on and remove the identical ones from the children

canova force-pushed the track-search branch from 3462260 to 8a759e5 Compare November 22, 2021 15:29

canova merged commit 8da8e85 into firefox-devtools:main Nov 22, 2021

canova deleted the track-search branch November 22, 2021 16:02

canova mentioned this pull request Nov 23, 2021

Prevent enter key to close the track context menu #3667

Merged

canova mentioned this pull request Jan 19, 2022

Add a search filter for timeline tracks #3574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a search filter to the timeline track context menu #3634

Add a search filter to the timeline track context menu #3634

canova commented Nov 2, 2021 •

edited

Loading

codecov bot commented Nov 2, 2021 •

edited

Loading

flodolo left a comment

canova commented Nov 4, 2021

julienw left a comment

julienw Nov 8, 2021

julienw Nov 8, 2021

canova Nov 17, 2021

julienw Nov 19, 2021 •

edited

Loading

julienw Nov 8, 2021

julienw Nov 8, 2021

julienw Nov 8, 2021

canova Nov 17, 2021

julienw Nov 8, 2021

canova Nov 17, 2021

canova left a comment

canova Nov 17, 2021

canova Nov 17, 2021

canova Nov 17, 2021

julienw left a comment

	requestAnimationFrame(() => {
	// Restore focus, but not during a React setState call, otherwise this triggers
	// a React warning:
	// "unstable_flushDiscreteUpdates: Cannot flush updates when React is already rendering."
	focusAfterClosed.focus();
	});

                 };
               }
+              /**

Add a search filter to the timeline track context menu #3634

Add a search filter to the timeline track context menu #3634

Conversation

canova commented Nov 2, 2021 • edited Loading

codecov bot commented Nov 2, 2021 • edited Loading

Codecov Report

flodolo left a comment

Choose a reason for hiding this comment

canova commented Nov 4, 2021

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

julienw Nov 19, 2021 • edited Loading

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

canova 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

julienw left a comment

Choose a reason for hiding this comment

canova commented Nov 2, 2021 •

edited

Loading

codecov bot commented Nov 2, 2021 •

edited

Loading

julienw Nov 19, 2021 •

edited

Loading