Add sort buttons (▲/▼) to TreeView columns

[parttimenerd]

I botched the rebase for #4203, so I created a new PR.

his change adds sorts buttons to the MarkerTable and CallTree, allowing to sort the columns of both views. An example: Before this change the marker table looked like:

After this change it looks initially like:

Making it clear to the user that sorting is available (and that the table is sorted by the first column). Clicking on the "Start" column changes this to:

Clicking on another column like "Duration" sorts by this column:

It includes tests for the expected behavior.

Deploy preview

[parttimenerd]

The relevant (and not already obsolete) comments from the PR #4203:

@canova:

Thanks for the PR! Also sorry about the delay on the review, I've been swamped with the reviews lately on top of work related stuff.

We've discussed this with Julien as well and have some feedback:

1. It looks like sorting isn't suitable for call tree due to its tree structure. So we would prefer to leave the call tree for now and only implement sorting for marker table.
2. Currently we are keeping sorting states for all the columns at the same time. We've looked at some of the examples on other applications and it looks like they are all keeping a single sorting state for a table. Currently this also brings some complexity to code as well. We can do the same thing what others doing. If you look at the examples as well, you'll see that it's mostly one triange at the same time that indicates which column is used for sorting. For example here's spotify: Screen Shot 2022-09-08 at 7 17 08 PM As you can see, there is only one arrow at the same time. This makes things clearer as well for the users.
3. Also while we were looking at other examples, we've seen that nearly all the programs use the arrow on the right side. See the example above for spotify, but it's also the same for programs like excel or google spreadsheets. So it would nice for us to do the same as well.
4. Since we are not going to deal with call tree, we can move the complexity inside TreeView to the parent component as well. So that way we don't have to deal with the complexity of the tree and we can just deal with an array of markers instead. See my comment below for details too. We can probably handle this inside getMarkerTree.

Let me know what you think, thanks!

@mstange:

I think it makes sense to have this functionality in the TreeView. The method list view is another table where users might want to change which column we sort by, they could switch between sorting by total time and sorting by self time. And in the call tree, if you're looking at a diff profile, you may want to reverse the order.

I agree with Nazim that the arrow should be at the end and that only one arrow should be visible at any time (even if, internally, you use a lexicographical sort over multiple columns in the reversed clicked order).

Also, I'd like the visuals of the arrow and of the mousedown state to match the native macOS tree view, but I can tweak that after this PR lands.

[parttimenerd]

Changes:

• the call tree only sorts the root nodes (as discussed)
• the sorting button is shown on the right side (as suggested)
• only the sort button of column that was sorted last is shown
• suggested small changes regarding columns (now use strings) and constant sets were incorporated

[julienw]

Thanks, I find the UI much better than in the previous iteration. There's still a small weird behavior when switching columns, it looks like the column "remembers" the previous state, so for example we click on column A (it's ascending), then column B (it's ascending), then column A again (it's descending!! but I expect ascending in this case).

Otherwise about the sorting implementation itself, we discussed this a bit with Markus, and we agreed that the sorting doesn't happen at the right position with this PR. This shouldn't be in the TreeView component but in the Tree structure that governs how the data is displayed.

For example, this is where it should be for the marker table: https://github.com/firefox-devtools/profiler/blob/14d46cc0f4b6e92057fcc665b575e5473be00ae0/src/components/marker-table/index.js#L74.

And for the call tree (if we want to do it there): https://github.com/firefox-devtools/profiler/blob/14d46cc0f4b6e92057fcc665b575e5473be00ae0/src/profile-logic/call-tree.js#L152-L156 (note it would be recursive "for free" in the call tree with this change)

I think we can do it for the marker table first, as we discussed in the past, because it's probably much easier.

[parttimenerd]

Thanks, I find the UI much better than in the previous iteration. There's still a small weird behavior when switching columns, it looks like the column "remembers" the previous state, so for example we click on column A (it's ascending), then column B (it's ascending), then column A again (it's descending!! but I expect ascending in this case).

Oh, I forgot to reset it. I probably thought to complicated.

Otherwise about the sorting implementation itself, we discussed this a bit with Markus, and we agreed that the sorting doesn't happen at the right position with this PR. This shouldn't be in the TreeView component but in the Tree structure that governs how the data is displayed.

You mean the following?

interface Tree<DisplayData: Object> {
  getDepth(NodeIndex): number;
  getRoots(): NodeIndex[];
  getDisplayData(NodeIndex): DisplayData;
  getParent(NodeIndex): NodeIndex;
  getChildren(NodeIndex): NodeIndex[];
  hasChildren(NodeIndex): boolean;
  getAllDescendants(NodeIndex): Set<NodeIndex>;
 sort(string): Tree<DisplayData>;
}

[julienw]

Thanks, I find the UI much better than in the previous iteration. There's still a small weird behavior when switching columns, it looks like the column "remembers" the previous state, so for example we click on column A (it's ascending), then column B (it's ascending), then column A again (it's descending!! but I expect ascending in this case).

Oh, I forgot to reset it. I probably thought to complicated.

Otherwise about the sorting implementation itself, we discussed this a bit with Markus, and we agreed that the sorting doesn't happen at the right position with this PR. This shouldn't be in the TreeView component but in the Tree structure that governs how the data is displayed.

You mean the following?

interface Tree<DisplayData: Object> {
  getDepth(NodeIndex): number;
  getRoots(): NodeIndex[];
  getDisplayData(NodeIndex): DisplayData;
  getParent(NodeIndex): NodeIndex;
  getChildren(NodeIndex): NodeIndex[];
  hasChildren(NodeIndex): boolean;
  getAllDescendants(NodeIndex): Set<NodeIndex>;
 sort(string): Tree<DisplayData>;
}

I mean that the sorting should happen directly in getRoots and getChildren, probably using some arguments passed to the constructor.

The TreeView itself would be reponsible for displaying the sorting marker (like you did now I believe), and calling callbacks when the user clicks on them.

[parttimenerd]

I implement all suggestions, but I still have a test error that I don't understand (the code works when I test it manually in the browser).

[parttimenerd]

I fixed the small bug that caused the test to fail.

[parttimenerd]

Ping

[parttimenerd]

I rebased it on the current main branch.