Add support for multi row select and context menu in Timegraph Component #1044

ngondalia · 2024-02-01T22:58:03Z

Changes made:

Support for adding context menu via signals
Support for making multiple selections in data tree of timegraph component
Added Signals to notify context menu selection and multi row selections

This change will allow for a context menu to be added to the timegraph component based on the outputDescriptor id via a signal. The component also adds the ability to select multiple rows. The multi selections can be passed with the item clicked signal payload as well as through a rowSelectionsChanged signal. The end goal is to provide the timegraph views with the ability to make multi-row selections and perform some actions with the selections.

Signed-off-by: Neel Gondalia ngondalia@blackberry.com

ngondalia · 2024-02-02T19:19:11Z

In order to test this:

First register a context menu to timegraph component using the signal:

export const ctxMenu: ContextMenu = {
  menuId: "ctx.Menu",
  submenus: [
    {
      label: "Submenu 1",
      items: [
        {id: "SubmenuItem1", label: "Submenu Item 1"},
      ],
      submenu: undefined
    }
  ],
  items: [
    {id: "menuItem2", label: "Menu Item 2"}
  ]
};

//descriptor can be any timegraph output descriptor for which you want a context menu
signalManager().fireContributeContextMenu({outputDescriptor: descriptor, menu: ctxMenu});

Once you have done that - your timegraph view should have a context menu along with the ability to multi-select rows.
The way to multi-select is: "CTRL+click or standard Shift+click behaviour

Once you make selections, you can right click, and click a menu item. To handle the item clicked, you can attach a listener for CONTEXT_MENU_ITEM_CLICKED signal:

private _onContextMenuItemClicked = (payload: ContextMenuItemClickedSignalPayload): void =>  { // handle item clicked payload and perform whatever function you need to based on the selection };

signalManager().on(Signals.CONTEXT_MENU_ITEM_CLICKED, this._onContextMenuItemClicked);

ngondalia · 2024-02-02T19:24:15Z

ROW_SELECTIONS_CHANGED - This signal is intended for components that are interested in the multi selections but without the actions through a context menu

bhufmann · 2024-02-07T18:51:32Z

Thanks for the contribution. Sounds interesting. I see that such an API provides the possibility for third-party extensions to add custom functionality. To be able to use it also in vscode-trace-extension, there is some more work to be done there.

I tried something similar at one point, but I've never been able to work on it beyond a proof-of-concept. See below for both related branches on my GitHub forks. I'm not able to continue working on it in the near future, so, I'm glad that you're bringing this forward.

I'll review your PR in the coming days.

https://github.com/bhufmann/theia-trace-extension/tree/command-handler-api
https://github.com/bhufmann/vscode-trace-extension/tree/command-handler-api

ngondalia · 2024-02-09T01:44:23Z

Thanks for the contribution. Sounds interesting. I see that such an API provides the possibility for third-party extensions to add custom functionality. To be able to use it also in vscode-trace-extension, there is some more work to be done there.

I tried something similar at one point, but I've never been able to work on it beyond a proof-of-concept. See below for both related branches on my GitHub forks. I'm not able to continue working on it in the near future, so, I'm glad that you're bringing this forward.

I'll review your PR in the coming days.

https://github.com/bhufmann/theia-trace-extension/tree/command-handler-api https://github.com/bhufmann/vscode-trace-extension/tree/command-handler-api

Okay, Thanks! :)

Yes, there will have to be a follow up commit to this for vscode trace extension! I have it done, i will push the PR soon!

I briefly look at the work you did - I think a generic command handler API would definitely be very useful! I will look at it in detail tomorrow - maybe I can pick it up at some point.

ngondalia · 2024-02-14T18:50:11Z

Thanks for the contribution. Sounds interesting. I see that such an API provides the possibility for third-party extensions to add custom functionality. To be able to use it also in vscode-trace-extension, there is some more work to be done there.

I tried something similar at one point, but I've never been able to work on it beyond a proof-of-concept. See below for both related branches on my GitHub forks. I'm not able to continue working on it in the near future, so, I'm glad that you're bringing this forward.

I'll review your PR in the coming days.

https://github.com/bhufmann/theia-trace-extension/tree/command-handler-api https://github.com/bhufmann/vscode-trace-extension/tree/command-handler-api

Sorry for the delay: I have posted eclipse-cdt-cloud/vscode-trace-extension#205, which is the follow up work for this commit in the vscode trace extension. Looking forward to your reply!

bhufmann

Thanks a lot for this interesting contribution. My mind started thinking on how to use it for our user base. It opens some door of add-on features for specific views.

I did a pass of review and testing. I added my findings to this review.

bhufmann · 2024-02-27T13:55:07Z

packages/base/src/signals/context-menu-item-clicked-signal-payload.tsx

@@ -0,0 +1,29 @@
+/***************************************************************************************
+ * Copyright (c) 2023 BlackBerry Limited and contributors.


bhufmann · 2024-02-27T13:56:21Z

packages/base/src/signals/context-menu-contributed-signal-payload.tsx

@@ -0,0 +1,41 @@
+/***************************************************************************************
+ * Copyright (c) 2023 BlackBerry Limited and contributors.


bhufmann · 2024-02-27T18:35:05Z

packages/react-components/src/components/timegraph-output-component.tsx

+        let calculatedX = 0;
+        let calculatedY = 0;
+        if (refNode) {
+            calculatedX = event.clientX;


shouldn't it be calculatedX = event.clientX - refNode.getBoundingClientRect().left;?

Otherwise the menu is opened further right and it feel not correct, as well as the location is further left if the trace explorer is minimized.

I meant for it to opened further right as that is the flow of the view - but i am ok with what you suggested as well - I will update it in the next patch!

bhufmann · 2024-02-27T19:09:55Z

packages/react-components/src/components/timegraph-output-component.tsx

+                rows?.push(id);
+            }
+        }
+        this.setState({ selectedRow: undefined, selectedRows: [...rows] });


Does selectedRow to need to be set to undefined? When clicking afterwards on a context menu, the selected row is undefined. Also, there s a misalignment between the tree on the left and the state row on the right of the view.

I primiarily did this for styling of the table row.. I wanted all multi selection of rows to be the same color - and if i dont set selectedRow to be undefined, the className that gets attached to the row would be selected as that was my 2nd check for what className should be used.

But on second thought, I dont think multi-select needs to be a different background. It can just re-use the same background as the normal selection. WDYT?

bhufmann · 2024-02-27T19:47:23Z

packages/react-components/src/components/timegraph-output-component.tsx

@@ -977,6 +1094,12 @@ export class TimegraphOutputComponent extends AbstractTreeOutputComponent<Timegr
        }
    };

+    private doHandleContextMenuContributed(payload: ContextMenuContributedSignalPayload) {
+        if (payload.getOutputDescriptor().id === this.props.outputDescriptor.id) {
+            this.setState({ ctxMenu: payload.getMenu() });


if there are multiple menus contributed, then the last menu will override the previous one. What I can see that there might be multiple sources of menu contributions, e.g. external third-party extension (I think that's your case), maybe some build-in menus, or maybe some server driven-menus (haven't fully thought this through though), So, I don't think it we should be limiting it to one possible contribution.

Hmm good point - Maybe i should change it so that the signals can pass menu items rather an entire context menu - That way it will have a generic menu with the ability to add as many menu items as needed from multiple sources. WDYT?

bhufmann · 2024-02-27T19:55:26Z

packages/react-components/src/components/timegraph-output-component.tsx

+            selectedRow: this.state.selectedRow,
+            selectedRows: this.state.selectedRows
+        };
+        const signalPayload: ContextMenuItemClickedSignalPayload = new ContextMenuItemClickedSignalPayload(


each row might have some metadata associated which can be used on the receiving side of the signal. The metadata is aleady used to select a specific row upon reception of a selection changed signal from the events table (see method findElement()). It night be interesting to send the metadata as part of the this new signal and row_selection_changed_signal instead of just the array of row ids for future enhancements.

That would definitely be a very nice enhancement to this signal - I will look into it

bhufmann · 2024-02-27T20:00:25Z

packages/react-components/src/components/timegraph-output-component.tsx

+    protected handleItemClick = (params: ItemParams): void => {
+        const props = {
+            outputDescriptor: this.props.outputDescriptor,
+            selectedRow: this.state.selectedRow,


selectedRow and selectedRows are to close in name and name should be clearer. Maybe rename new variable and releated methods/signals to make it clearer.

Ok - i will change it in the next patch!

bhufmann · 2024-02-27T20:02:47Z

packages/react-components/src/components/utils/filter-tree/table-row.tsx

+            onMultipleRowClick(node.id, false);
+        } else if (onMultipleRowClick && e.shiftKey) {
+            onMultipleRowClick(node.id, true);
+        } else {


I noticed the following exception in the log:

Uncaught TypeError TypeError: onRowClick is not a function at TableRow.onClick (/home/eedbhu/git/theia-training/theia-trace-extension/packages/react-components/src/components/utils/filter-tree/table-row.tsx:102:13) at callCallback (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:4164:1) at invokeGuardedCallbackDev (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:4213:1) at invokeGuardedCallback (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:4277:1) at invokeGuardedCallbackAndCatchFirstError (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:4291:1) at executeDispatch (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9041:1) at processDispatchQueueItemsInOrder (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9073:1) at processDispatchQueue (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9086:1) at dispatchEventsForPlugins (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9097:1) at <anonymous> (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9288:1) at batchedUpdates$1 (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:26140:1) at batchedUpdates (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:3991:1) at dispatchEventForPluginEventSystem (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:9287:1) at dispatchEventWithEnableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:6465:1) at dispatchEvent (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:6457:1) at dispatchDiscreteEvent (/home/eedbhu/git/theia-training/theia-trace-extension/node_modules/react-dom/cjs/react-dom.development.js:6430:1)

I can't seem to reproduce this - can you maybe provide some steps to reproduce this?

bhufmann · 2024-02-27T20:05:49Z

packages/react-components/src/components/timegraph-output-component.tsx

+    };
+
+    renderContextMenu(): React.ReactNode {
+        return (


right mouse click on a row of a timegraph view that doesn't have a context menu registered I get the following error in the console log (descriptor id is the one I used and will vary for other data providers):

root ERROR It seems that the menu you are trying to display is not renderer or you have a menu id mismatch. You used the menu id: timegraph.context.menuId org.eclipse.tracecompass.incubator.internal.callstack.core.instrumented.provider.flamechart:org.eclipse.tracecompass.incubator.callstack.core.lttng.ust```

Ah - I forgot to add a check in onCtxMenu to see if a context menu was contributed - will fix in next patch!

bhufmann · 2024-02-27T20:13:24Z

packages/base/src/signals/context-menu-contributed-signal-payload.tsx

+    private menu: ContextMenu;
+
+    constructor(outputDescriptor: OutputDescriptor, menuItems: ContextMenu) {
+        this.outputDescriptor = outputDescriptor;


I'm a bit torn about the following comment. I'm not sure if the whole descriptor needs to be sent or just the descriptor ID. The sender of that signal might not know all the mandatory fields of the descriptor. But it might be useful to have the descriptor. WDYT?

I don't think the whole descriptor is needed - just the descriptor id would suffice for the purpose of attaching a context menu. But I left in the whole OutputDescritor incase you had a use case for it

changes made: - Support for adding context menu via signals - Support for making multiple selections in data tree of timegraph component - Added Signals to notify context menu selection and multi row selections This change will allow for a context menu to be added to the timegraph component based on the outputDescriptor id via a signal. The component also adds the ability to select multiple rows. The multi selections can be passed with the item clicked signal payload as well as through a rowSelectionsChanged signal. The end goal is to provide the timegraph views with the ability to make multi-row selections and perform some actions with the selections. Signed-off-by: Neel Gondalia ngondalia@blackberry.com

ngondalia · 2024-03-04T22:02:08Z

I have addressed most of the issues from the previous patch - I have also updated eclipse-cdt-cloud/vscode-trace-extension#205, which is the code i used to test this from vscode-trace-extension.

I looked into updating the styling of chart layer rows - but currently only way to update a row's style is by updating selectedRow, which doesnt work for multi selection rows which invokes rowStyleProvider through "updateRowStyle". To support this, it requires a change in Timeline Chart, which includes making "updateRowStyle" method in time-graph-chart.ts to be public API. However, we do not have an internal mirror/setup for that repo, so i wont be able to commit to it anytime soon.

The workaround would be to update chart layer anytime the multiselections are changed - but i dont think that is a good idea as it is a costly operation and it breaks the user flow.. WDYT?

bhufmann · 2024-03-12T17:05:47Z

Thanks for the updates. I going to test and review your updates soon.

bhufmann · 2024-03-13T19:54:39Z

I have addressed most of the issues from the previous patch - I have also updated eclipse-cdt-cloud/vscode-trace-extension#205, which is the code i used to test this from vscode-trace-extension.

I looked into updating the styling of chart layer rows - but currently only way to update a row's style is by updating selectedRow, which doesnt work for multi selection rows which invokes rowStyleProvider through "updateRowStyle". To support this, it requires a change in Timeline Chart, which includes making "updateRowStyle" method in time-graph-chart.ts to be public API. However, we do not have an internal mirror/setup for that repo, so i wont be able to commit to it anytime soon.

The workaround would be to update chart layer anytime the multiselections are changed - but i dont think that is a good idea as it is a costly operation and it breaks the user flow.. WDYT?

I think it's ok to only synchronize one single selection between tree and chart. This new feature of this PR then will apply only for the single- or multi-selection in the tree. If there is a need for a different behaviour in the future then we can address it.

bhufmann

It looks good to me.

I tested it with a sample code within this repo and with example eclipse-cdt-cloud/vscode-trace-extension#205.

Thanks for the contribution!

ngondalia force-pushed the multiRowSelectionAndCtxMenu branch from eea555b to a3eb3ef Compare February 2, 2024 18:38

ngondalia force-pushed the multiRowSelectionAndCtxMenu branch from a3eb3ef to 6c97f0a Compare February 2, 2024 19:25

bhufmann self-requested a review February 7, 2024 15:08

ngondalia mentioned this pull request Feb 14, 2024

Handle new signal for context menus and multi-row selection eclipse-cdt-cloud/vscode-trace-extension#205

Merged

bhufmann requested changes Feb 27, 2024

View reviewed changes

ngondalia force-pushed the multiRowSelectionAndCtxMenu branch from 6c97f0a to 50b4409 Compare March 4, 2024 21:39

ngondalia requested a review from bhufmann March 4, 2024 21:41

ngondalia force-pushed the multiRowSelectionAndCtxMenu branch 2 times, most recently from fa4f7bd to da7b720 Compare March 4, 2024 21:56

ngondalia force-pushed the multiRowSelectionAndCtxMenu branch from da7b720 to 75aa1c0 Compare March 4, 2024 21:59

bhufmann approved these changes Mar 13, 2024

View reviewed changes

ngondalia merged commit d1616c4 into eclipse-cdt-cloud:master Mar 13, 2024
4 checks passed

ngondalia deleted the multiRowSelectionAndCtxMenu branch March 13, 2024 20:34

marcdumais-work mentioned this pull request Mar 19, 2024

Test publish: latest (v0.2.0) npm packages #1058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multi row select and context menu in Timegraph Component #1044

Add support for multi row select and context menu in Timegraph Component #1044

ngondalia commented Feb 1, 2024

ngondalia commented Feb 2, 2024 •

edited

Loading

ngondalia commented Feb 2, 2024

bhufmann commented Feb 7, 2024

ngondalia commented Feb 9, 2024 •

edited

Loading

ngondalia commented Feb 14, 2024

bhufmann left a comment

bhufmann Feb 27, 2024

bhufmann Feb 27, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024 •

edited

Loading

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024

bhufmann Feb 27, 2024

ngondalia Feb 29, 2024 •

edited

Loading

ngondalia commented Mar 4, 2024 •

edited

Loading

bhufmann commented Mar 12, 2024

bhufmann commented Mar 13, 2024

bhufmann left a comment

		@@ -0,0 +1,29 @@
		/***************************************************************************************
		* Copyright (c) 2023 BlackBerry Limited and contributors.

		@@ -0,0 +1,41 @@
		/***************************************************************************************
		* Copyright (c) 2023 BlackBerry Limited and contributors.

Add support for multi row select and context menu in Timegraph Component #1044

Add support for multi row select and context menu in Timegraph Component #1044

Conversation

ngondalia commented Feb 1, 2024

ngondalia commented Feb 2, 2024 • edited Loading

ngondalia commented Feb 2, 2024

bhufmann commented Feb 7, 2024

ngondalia commented Feb 9, 2024 • edited Loading

ngondalia commented Feb 14, 2024

bhufmann 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

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

ngondalia Feb 29, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngondalia Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

ngondalia commented Mar 4, 2024 • edited Loading

bhufmann commented Mar 12, 2024

bhufmann commented Mar 13, 2024

bhufmann left a comment

Choose a reason for hiding this comment

ngondalia commented Feb 2, 2024 •

edited

Loading

ngondalia commented Feb 9, 2024 •

edited

Loading

ngondalia Feb 29, 2024 •

edited

Loading

ngondalia Feb 29, 2024 •

edited

Loading

ngondalia commented Mar 4, 2024 •

edited

Loading