Implement the shift click gesture to select several adjacent threads in the timeline by julienw · Pull Request #4138 · firefox-devtools/profiler

julienw · 2022-07-11T18:17:24Z

Gestures that work:

click some track, shift click some other track => threads in-between are selected. This works in both directions.
click some track, ctrl click some other track, shift + click a third track => the range is added to the initial selection
at load time, when there's just one selected track, it's possible to shift + click another track, the existing selected track will be used as the start of the range
for all of these gestures, after a shift-click, we can shift-click elsewhere, which should cancel the previous shift-click and select again.

Here is a deploy preview

Fixes #2710

Screencast_20220721_184835.mp4

codecov · 2022-07-18T15:56:26Z

Codecov Report

Merging #4138 (61d33ae) into main (aefb4d3) will increase coverage by 0.13%.
The diff coverage is 94.20%.

❗ Current head 61d33ae differs from pull request most recent head 7caaf80. Consider uploading reports for the commit 7caaf80 to get more accurate results

@@            Coverage Diff             @@
##             main    #4138      +/-   ##
==========================================
+ Coverage   88.33%   88.46%   +0.13%     
==========================================
  Files         280      280              
  Lines       24409    24510     +101     
  Branches     6491     6533      +42     
==========================================
+ Hits        21561    21683     +122     
+ Misses       2645     2625      -20     
+ Partials      203      202       -1

Impacted Files	Coverage Δ
src/components/timeline/LocalTrack.js	`73.17% <ø> (+1.21%)`	⬆️
src/components/timeline/TrackThread.js	`97.18% <92.85%> (+4.97%)`	⬆️
src/actions/profile-view.js	`87.18% <93.00%> (+3.90%)`	⬆️
src/profile-logic/tracks.js	`83.45% <94.11%> (+0.35%)`	⬆️
src/components/timeline/GlobalTrack.js	`70.64% <100.00%> (ø)`
src/reducers/profile-view.js	`96.21% <100.00%> (+0.35%)`	⬆️
src/selectors/profile.js	`97.00% <100.00%> (+<0.01%)`	⬆️
src/utils/index.js	`88.57% <100.00%> (ø)`
src/components/app/ServiceWorkerManager.js	`84.82% <0.00%> (-4.66%)`	⬇️
src/index.js	`0.00% <0.00%> (ø)`
... and 6 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 aefb4d3...7caaf80. Read the comment docs.

mstange · 2022-07-23T17:20:05Z

This works quite well!

I've noticed one case that I think can be tweaked: "Shift selection after Cmd selection"

Have thread 1 selected.
Cmd+click thread 3.
Shift+click thread 5.

Expected selection: 1, 3, 4, 5 (this is what macOS Finder does)
Actual selection: 3, 4, 5 (everything outside of the new shift selection got discarded)

mstange · 2022-07-23T17:24:37Z

I also noticed some weirdness with the IPC tracks, but that's probably something that can be fixed as a follow-up.

For example, in the "Utility AudioDecoder" process, the "MediaPDecoder #1" thread is framed by two IPC tracks. Clicking one of those IPC tracks and then shift-clicking the other creates a selection which only contains the thread-track in between. Cmd+clicking the tracks individually has a different result.

julienw · 2022-07-23T22:53:43Z

This works quite well!

I've noticed one case that I think can be tweaked: "Shift selection after Cmd selection"
1. Have thread 1 selected.

2. Cmd+click thread 3.

3. Shift+click thread 5.
Expected selection: 1, 3, 4, 5 (this is what macOS Finder does) Actual selection: 3, 4, 5 (everything outside of the new shift selection got discarded)

Ah, that's interesting!
This works if you keep Ctrl pressed for the shift, but making it work without that would require saving the previous modifier, which sounds more work for this small use case...

I also noticed some weirdness with the IPC tracks, but that's probably something that can be fixed as a follow-up.

For example, in the "Utility AudioDecoder" process, the "MediaPDecoder #1" thread is framed by two IPC tracks. Clicking one of those IPC tracks and then shift-clicking the other creates a selection which only contains the thread-track in between. Cmd+clicking the tracks individually has a different result.

Ah that's interesting too! This is quite similar to the previous issue in that when we don't use Ctrl we replace the previous selection. I can see how it's weird in this case though and I'll think about how to fix it easily.

julienw · 2022-07-25T08:39:54Z

This works quite well!
I've noticed one case that I think can be tweaked: "Shift selection after Cmd selection"
1. Have thread 1 selected.

2. Cmd+click thread 3.

3. Shift+click thread 5.
Expected selection: 1, 3, 4, 5 (this is what macOS Finder does) Actual selection: 3, 4, 5 (everything outside of the new shift selection got discarded)
Ah, that's interesting! This works if you keep Ctrl pressed for the shift, but making it work without that would require saving the previous modifier, which sounds more work for this small use case...

Interestingly: libreoffice spreadsheets works like MacOS X' finder, but google spreadsheets works like my implementation.
Now I'm on the verge of implementing this :)

julienw · 2022-07-26T16:58:12Z

 Ah that's interesting too! This is quite similar to the previous issue in that when we don't use Ctrl we replace the previous selection. I can see how it's weird in this case though and I'll think about how to fix it easily.

That one was easy to fix.

I'm trying to implement the other behavior.

mstange · 2022-07-27T13:49:40Z

Now this case doesn't work as expected anymore:

1 is selected, Cmd 3, Shift 5, Shift 2, expected: 1, 2, 3

I haven't looked at your code at all, but I would implement it as follows:

type State = {|
  currentSelection: Set<TrackId>,
  selectionAtLastNonShiftClick: Set<TrackId>,
  clickedTrackAtLastNonShiftClick: TrackId,
|};

function onClick(state: State, clickedTrack: TrackId): State {
  const newSelection = new Set([clickedTrack]);
  return {
    currentSelection: newSelection,
    selectionAtLastNonShiftClick: newSelection,
    clickedTrackAtLastNonShiftClick: clickedTrack,
  }
}

function onCmdClick(state: State, clickedTrack: TrackId): State {
  const newSelection = new Set([...state.currentSelection, clickedTrack]);
  return {
    currentSelection: newSelection,
    selectionAtLastNonShiftClick: newSelection,
    clickedTrackAtLastNonShiftClick: clickedTrack,
  }
}

function onShiftClick(state: State, clickedTrack: TrackId): State {
  const shiftSelection = computeTracksBetween(state.clickedTrackAtLastNonShiftClick, clickedTrack);
  const newSelection = new Set([...state.selectionAtLastNonShiftClick, ...shiftSelection]);
  return {
    ...state,
    currentSelection: newSelection,
  }
}

mstange · 2022-07-27T13:55:29Z

Now this case doesn't work as expected anymore:

Oh, correction, it wasn't working at all before either. So I should rather say: Thank you for implementing the Cmd-then-shift behavior, but here's a case that can still be improved to match macOS Finder more closely.

julienw · 2022-07-27T16:38:32Z

Now this case doesn't work as expected anymore:

1 is selected, Cmd 3, Shift 5, Shift 2, expected: 1, 2, 3

I agree, I expect this to work too, and it doesn't. This works as expected without the Ctrl thing.

I haven't looked at your code at all, but I would implement it as follows:

type State = {|
  currentSelection: Set<TrackId>,
  selectionAtLastNonShiftClick: Set<TrackId>,
  clickedTrackAtLastNonShiftClick: TrackId,
|};

function onClick(state: State, clickedTrack: TrackId): State {
  const newSelection = new Set([clickedTrack]);
  return {
    currentSelection: newSelection,
    selectionAtLastNonShiftClick: newSelection,
    clickedTrackAtLastNonShiftClick: clickedTrack,
  }
}

function onCmdClick(state: State, clickedTrack: TrackId): State {
  const newSelection = new Set([...state.currentSelection, clickedTrack]);
  return {
    currentSelection: newSelection,
    selectionAtLastNonShiftClick: newSelection,
    clickedTrackAtLastNonShiftClick: clickedTrack,
  }
}

function onShiftClick(state: State, clickedTrack: TrackId): State {
  const shiftSelection = computeTracksBetween(state.clickedTrackAtLastNonShiftClick, clickedTrack);
  const newSelection = new Set([...state.selectionAtLastNonShiftClick, ...shiftSelection]);
  return {
    ...state,
    currentSelection: newSelection,
  }
}

This is pretty close to what I did but maybe I added piles on top of other piles and I could now possibly simplify this a bit. For example my code used to work in a quite stateless way initially, always recomputing the full range when shift was used and replacing the full selection, but then I added more state to control it. I like how you wrote that shift click reuse the previous selection, which builds on the assumption that the previous click either already replaced the previous selection or added to the previous selection, and I haven't thought of this idea before. So thanks!

julienw · 2022-07-27T16:58:35Z

OK, I know what my bug is, and what you propose (saving the selectionAtLastNonShiftClick) is indeed the right way to implement this. My code is always adding selections during this STR, while we want to replace what was added instead.
Thanks for the feedback!

julienw · 2022-07-29T15:48:40Z

-          invertCallstack,
-          selectedThreadIndexes,
-        } = this.props;
+    const modifiers = getTrackSelectionModifiers(event);


Here it looks like it's a lot of changes, but if you look at the changes without the whitespace changes you'll see the code has just been reordered.

julienw · 2022-07-29T16:58:57Z

+ * information in the state. Because of all possible cases this isn't trivial.
+ * This will return null for tracks that are not selectable.
+ */
+function getInformationFromTrackReference(


This commit might be easier to follow with whitespace changes.

julienw · 2022-07-29T17:04:55Z

I reimplemented most of the patch following Markus' excellent suggestion (I wish I thought of that earlier :D), now I believe all mentioned use cases work as expected.

It should be easier to look commit by commit.

julienw · 2022-07-30T21:00:03Z

+    let selectedTab =
+      clickedTrackInformation.relatedTab ?? getSelectedTab(getState());
+    const visibleTabs = getThreadSelectors(
+      clickedTrackInformation.relatedThreadIndex
+    ).getUsefulTabs(getState());
    if (!visibleTabs.includes(selectedTab)) {
      // If the user switches to another track that doesn't have the current
      // selectedTab then switch to the first tab.
      selectedTab = visibleTabs[0];
    }


I wasn't sure what to do with the selectedTab, should we change it for all modes? Maybe we'd want to change it only for setOneTrackSelection... I thought it would be easy to change in a follow-up if needed.

canova

Looks great, thanks for the feature and adding lots of tests! I've added a few nits mostly.

canova · 2022-08-03T12:12:19Z

+      }
+    }
+
+    // Add the global track if it's not a virtual track and not out of the range.


By "virtual" you mean a global tack with a thread, right?

"virtual track" means here a global track that is not a thread (such as what we get when importing sometimes).

But the comment says "add the global track if it's not virtual", maybe that's where you got confused?

… to accept shift modifiers

…rence to a separate function

…ed track

…saved track

…ifiers

julienw · 2022-08-03T14:04:46Z

Thanks so much for the review! I believe I handled all your comments and will merge now!

julienw force-pushed the support-shift-click-in-timeline branch 4 times, most recently from b861724 to b6e8f9a Compare July 18, 2022 15:50

julienw force-pushed the support-shift-click-in-timeline branch 2 times, most recently from 94e4d4c to ea45ec6 Compare July 21, 2022 16:45

julienw marked this pull request as ready for review July 21, 2022 16:45

julienw requested a review from canova July 21, 2022 16:45

julienw force-pushed the support-shift-click-in-timeline branch from ea45ec6 to 33fcee5 Compare July 21, 2022 16:55

julienw removed the request for review from canova July 21, 2022 16:57

julienw force-pushed the support-shift-click-in-timeline branch 5 times, most recently from 3fd5ecd to 5f305b2 Compare July 22, 2022 14:28

julienw requested a review from canova July 22, 2022 14:33

julienw removed the request for review from canova July 25, 2022 09:53

julienw force-pushed the support-shift-click-in-timeline branch from 5f305b2 to 1ab1c13 Compare July 26, 2022 16:51

julienw force-pushed the support-shift-click-in-timeline branch from 1ab1c13 to 964e667 Compare July 27, 2022 13:08

julienw mentioned this pull request Jul 27, 2022

Fix an issue where a local track wasn't unselected when its global track is hidden #4158

Merged

julienw force-pushed the support-shift-click-in-timeline branch 2 times, most recently from 5438b44 to a267764 Compare July 27, 2022 14:29

julienw force-pushed the support-shift-click-in-timeline branch 2 times, most recently from 5b85355 to 61d33ae Compare July 27, 2022 16:21

julienw force-pushed the support-shift-click-in-timeline branch from 61d33ae to 3cc7a3c Compare July 29, 2022 15:39

julienw commented Jul 29, 2022

View reviewed changes

julienw force-pushed the support-shift-click-in-timeline branch 4 times, most recently from a92915d to 71f7278 Compare July 29, 2022 16:33

julienw commented Jul 29, 2022

View reviewed changes

julienw requested a review from canova July 29, 2022 17:04

julienw commented Jul 30, 2022

View reviewed changes

canova approved these changes Aug 3, 2022

View reviewed changes

julienw added 9 commits August 3, 2022 16:00

Change the structure of the return value of getTrackSelectionModifier…

fa5ea9e

… to accept shift modifiers

Extract the logic to find information about tracks from the TrackRefe…

2e43d7c

…rence to a separate function

Extract the various modes of selecting a track to separate functions

4b6bb89

Save the information about the last clicked track to the redux state

ef627ad

Implement the shift + click gesture to select a range of threads

1389ecf

Invert 'from' and 'to' tracks when the clicked track is above the sav…

ca37dd9

…ed track

Find the track reference from the selected tracks if we don't have a …

cc64dbc

…saved track

Remove the 'last clicked track' information when it's hidden

ddb7f1b

Review: change a few comments and extract a type for the keyboard mod…

7caaf80

…ifiers

julienw force-pushed the support-shift-click-in-timeline branch from 71f7278 to 7caaf80 Compare August 3, 2022 14:01

julienw enabled auto-merge August 3, 2022 14:05

julienw merged commit 192554a into firefox-devtools:main Aug 3, 2022

Conversation

julienw commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mstange commented Jul 23, 2022

Uh oh!

mstange commented Jul 23, 2022

Uh oh!

julienw commented Jul 23, 2022

Uh oh!

julienw commented Jul 25, 2022

Uh oh!

julienw commented Jul 26, 2022

Uh oh!

mstange commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstange commented Jul 27, 2022

Uh oh!

julienw commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienw commented Jul 27, 2022

Uh oh!

julienw Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

julienw commented Jul 29, 2022

Uh oh!

julienw Jul 30, 2022

Choose a reason for hiding this comment

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

canova Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

julienw Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julienw commented Aug 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

julienw commented Jul 11, 2022 •

edited

Loading

codecov Bot commented Jul 18, 2022 •

edited

Loading

mstange commented Jul 27, 2022 •

edited

Loading

julienw commented Jul 27, 2022 •

edited

Loading

julienw Jul 29, 2022 •

edited

Loading