Queries panel: Expand/collapse child nodes

[shati-patel]

Small follow-up to #2418, to handle expanding/collapsing nodes in the Queries panel. (Still using mock data for now.)

Checklist

N/A—internal only ✨ 🦄

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[charisk]

Would it make sense to apply this logic when creating QueryTreeViewItems? I think we do something similar in the db tree view items.

[robertbrignull]

Correct me if I'm wrong here, but I think what @charisk means is we want to store ourselves whether the item is expanded or not. We could store the expanded state in the QueryTreeViewItem type and then just return it here.

The reason we want to do this is that if the user expands an item but then the table gets re-rendered we want to keep that expanded state and not collapse all items. The table could get re-rendered if we detect changes to the file system, but also could likely just happen semi-randomly for reasons outside of our control so we just need to support it generally. It also opens up the option for having the same items expanded when you re-open vscode, but we don't have to implement that right away.

This does mean we need to know when the user expands/collapses items, and we can do that using the TreeView.onDidExpandElement listener. I had a hacky go at implementing this just to check it'll work, so I'll post that code here. You're welcome to use it or not as you please.

And just to make clear, we could try implementing this in this PR, or save it for another. And I'm happy to talk through this more in person. Sorry I didn't realise that handling expanded items would actually be a bit complicated.

---

Implement setCollapsibleState on QueryTreeDataProvider:

public setCollapsibleState(
  element: QueryTreeViewItem,
  state: vscode.TreeItemCollapsibleState,
  node?: QueryTreeViewItem,
): void {
  if (node === undefined) {
    this.queryTreeItems.forEach((node) => {
      this.setCollapsibleState(element, state, node);
    });
  } else if (node === element) {
    node.expandedState = state;
  } else {
    node.children.forEach((child) => {
      this.setCollapsibleState(element, state, child);
    });
  }
}

This is a little bit messy because we need to handle starting at the root and then recursing through the tree to find the right element. It could likely be made a bit cleaner.

And add listeners to the tree view in QueriesPanel:

this.treeView.onDidExpandElement((event) => {
  this.dataProvider.setCollapsibleState(
    event.element,
    vscode.TreeItemCollapsibleState.Expanded,
  );
});
this.treeView.onDidCollapseElement((event) => {
  this.dataProvider.setCollapsibleState(
    event.element,
    vscode.TreeItemCollapsibleState.Collapsed,
  );
});

[robertbrignull]

Actually, doing node === element won't work because they're different objects. I'd forgotten in getTreeItem we make a new object.

So we need some way to know which TreeItem corresponds to which QueryTreeViewItem. The way to do this would be the TreeItem.id field, and add an equivalent id field to QueryTreeViewItem. The ID could be the absolute file path of the query file, so it's something that is easy to calculate and won't change.

Looking at this TreeItem.id field, it says:

/**
 * Optional id for the tree item that has to be unique across tree. The id is used to preserve the selection and expansion state of the tree item.
 *
 * If not provided, an id is generated using the tree item's label. **Note** that when labels change, ids will change and that selection and expansion state cannot be kept stable anymore.
 */

So maybe it also sneakily handles keeping expanded/collapsed elements stable for us? We probably still want to set the id field so it's not autogenerated from the label, because the label won't be unique enough or 100% consistent. But if we do set the ID we could see if it just works and expanded state stays consistent.

[shati-patel]

The reason we want to do this is that if the user expands an item but then the table gets re-rendered we want to keep that expanded state and not collapse all items.

Ah yes, now I remember why we did all the fancy "expanded state" stuff in the DB panel! I will take a look at your code and the existing DB panel stuff and try implementing that here 🔍 Thank you 🙇🏽‍♀️

[charisk]

For the DB panel we had to keep track of the expanded state because we're rendering the tree based on a config file, so every time we update the config file we didn't want the user's expanded state to be lost (e.g. all items to get collapsed).

I don't think we have this challenge with the queries panel so I think the framework should be able to handle things for you.

What I meant with my original comment is that we could, if we wanted to, move the logic you've got here (that expands nodes that have children) into the constructor of QueryTreeViewItems.

There's a chance I've completely missed the mark here -- happy to jump on a call and clear things out.

[robertbrignull]

For the DB panel we had to keep track of the expanded state because we're rendering the tree based on a config file, so every time we update the config file we didn't want the user's expanded state to be lost (e.g. all items to get collapsed).

I don't think we have this challenge with the queries panel so I think the framework should be able to handle things for you.

You know more than me about what the framework supports, so I bow to your knowledge here. What I was thinking about was that we'll have to react in a very similar way to changes to the file system. If anything changes like a new file being added, then we'll need to re-draw the table, and we don't want the state to be lost. Will it handle that for us automatically?

[robertbrignull]

I also see there's a lot still to go on this area. I see there's the WIP branch you did, which implements the QueryDiscovery. So with that a lot of this will change, so perhaps we shouldn't get hung up on details here. I'm very happy for this discussion to not be blocking and just keep iterating. I'm happy with whatever @shati-patel
and @charisk want to do.

[charisk]

What I was thinking about was that we'll have to react in a very similar way to changes to the file system

That's a good point, I didn't think about that. I think it depends what the exact flow will be as to whether you'd want to keep track of expanded state, so worth leaving it out for now.

Also happy to go ahead with the current state or with moving to the ctor. Whatever works really. It's an area that will evolved so it doesn't matter too much.

[robertbrignull]

Since QueryTreeViewItem does have a constructor, we could use it here and rewrite the createTree function as

    return [
      new QueryTreeViewItem("name1", "path1", []),
      new QueryTreeViewItem("name2", "path2", [
        new QueryTreeViewItem("name3", "path3", []),
        new QueryTreeViewItem("name4", "path4", [
          new QueryTreeViewItem("name5", "path5", []),
        ]),
      ]),
    ];

This is also nice because it means if we add extra logic to the QueryTreeViewItem constructor then it'll get called. Currently we kinda bypassing the constructor which could cause confusion.

[shati-patel]

I've got myself a bit confused now, but I've pushed a commit that moves the collapsibleState logic into the constructor for QueryTreeViewItem 😅 Is that roughly what we want?

[robertbrignull]

Yeah sorry it got a bit much. This is fine for me right now. Let's just carry on implementing / iterating and we'll make any more changes as we go.