search

firefox-devtools / profiler

Firefox Profiler — Web app for Firefox performance analysis
https://profiler.firefox.com
Mozilla Public License 2.0
1.14k stars 372 forks source link

Allow resizing the selected range past the other bound of the current selected range #5031

Closed fqueze closed 3 days ago

fqueze commented 1 week ago

Production | Deploy preview

Currently when a profile has a selected range, dragging timelineSelectionGrippyRangeStart / timelineSelectionGrippyRangeEnd to adjust the start/end of the selected range has these behaviors that I find unpleasant:

The PR I'm submitting changes the behavior so that the start can be dragged past the current end and the end can be dragged past the current start. In these cases, the start/end are swapped so that the new selected range still has a start time < its end time.

For context, the use case I had was trying to slice a profile in two subprofiles. I first selected one half of the profile, committed the range, and shared a short url. Then I uncommited the range, which resulted in the previously committed range becoming the selected range. At that point, I wanted to select a new range where the start of the selected range would be the start of the profile, and the new end of the selected range would be the start of the previously selected range. I couldn't find a way to do it, and ended up doing a new selection, with an approximate end time. This PR makes this use case easy, without IMHO regressing anything useful (I can't think of a case where the existing behaviors would be more useful).

I expected to need to adjust tests for this new behavior, but nothing failed. I'm afraid this part of the code has very little test coverage, if any.

mstange commented 1 week ago

I like the behavior change.

One thing I noticed is that the dragging class is set on the wrong grippy after you "cross over", so the wrong grippy has the darker gray fill. Doesn't really matter though.

mstange commented 1 week ago

You could fix it by storing a dragHandlesAreSwapped boolean on the preview selection state.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.47%. Comparing base (b7fe972) to head (fdd42aa).

:exclamation: Current head fdd42aa differs from pull request most recent head ad1dd1d

Please upload reports for the commit ad1dd1d to get more accurate results.

Files Patch % Lines
src/components/timeline/Selection.js 0.00% 4 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #5031 +/- ## ========================================== - Coverage 88.49% 88.47% -0.03% ========================================== Files 304 304 Lines 27418 27405 -13 Branches 7410 7407 -3 ========================================== - Hits 24264 24247 -17 - Misses 2932 2933 +1 - Partials 222 225 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

julienw commented 4 days ago

I expected to need to adjust tests for this new behavior, but nothing failed. I'm afraid this part of the code has very little test coverage, if any.

There's a start of coverage for this code at the very end of components/Timeline.test.js, but this needs a lot of expansion to test your new code, so I won't ask you to do it unless you want to.