Sync event timestamp with timegraph #207
Conversation
|
@muddana-satish I currently reviewing this patch. I'll get back to you soon. |
packages/base/src/signal-manager.ts
Outdated
| @@ -3,6 +3,7 @@ import { EventEmitter } from 'events'; | |||
| export declare interface SignalManager { | |||
|
|
|||
| fireTooltipSignal(tooltip: { [key: string]: string }): void; | |||
| fireEventClickSignal(payload: {[timeStamp: string]: number }): void; | |||
There was a problem hiding this comment.
"timestamp" should be a different name. Timestamp is one of the keys. Later on we might have multiple once.
I wonder if we should pass strings instead of numbers and the signal receivers have to convert them if necessary.
Name could be fireSelectionChangedSignal(), selection changed (see comment below on signal name)
packages/base/src/signal-manager.ts
Outdated
| @@ -12,7 +13,8 @@ export const Signals = { | |||
| EXPERIMENT_OPENED: 'experiment opened', | |||
| EXPERIMENT_CLOSED: 'experiment closed', | |||
| EXPERIMENT_SELECTED: 'experiment selected', | |||
| TOOLTIP_UPDATED: 'tooltip updated' | |||
| TOOLTIP_UPDATED: 'tooltip updated', | |||
| EVENT_CLICKED: 'Event clicked' | |||
There was a problem hiding this comment.
Event -> event: should lower case. I wonder if we will re-use this signal for other selections changes (e.g. chart to table). So, maybe we call it "selection changed".
| > | ||
| </AgGridReact> | ||
| </div>; | ||
| } | ||
|
|
||
| private onEventClick(event: CellClickedEvent) { | ||
| const columns = event.columnApi.getAllColumns(); | ||
| const timeStampHeader = columns.find(column => column.getColDef().headerName === "Timestamp (ns)"); |
There was a problem hiding this comment.
The column header string is acutally "Timestamp ns" in the trace compass server.
There was a problem hiding this comment.
If we need this, then it's a sign there's a problem with the TSP, this is the kind of string that should never be hard-coded. Time to work on eclipse-cdt-cloud/trace-server-protocol#35?
| @@ -86,6 +87,7 @@ export class TimegraphOutputComponent extends AbstractTreeOutputComponent<Timegr | |||
| } | |||
| this.onElementSelected(this.selectedElement); | |||
| }); | |||
| signalManager().on(Signals.EVENT_CLICKED, ({ timeStamp }) => this.eventClicked(timeStamp)); | |||
There was a problem hiding this comment.
use payload instead of timestamp
| @@ -141,6 +143,19 @@ export class TimegraphOutputComponent extends AbstractTreeOutputComponent<Timegr | |||
| this.setState({ collapsedNodes: newList }); | |||
| } | |||
|
|
|||
| private eventClicked(timeStampObj: { [key: string]: number }) { | |||
There was a problem hiding this comment.
use payload instead of timestamp.
| start: newViewRangeStart, | ||
| end: newViewRangeStart | ||
| }; | ||
| this.chartLayer.setEventClickedFlag(true); |
There was a problem hiding this comment.
I would prefer to separate the timestamp sync and row sync in 2 different pull requests (in theia-trace-extesion and timeline-chart).
|
Should be linked to #111 |
|
@bhufmann Modified as per your suggestions. Now the selection cursor moves based on the timestamp from the event row. |
ghost
changed the title
ghost
self-assigned this
ghost
requested a review
from
packages/base/src/signal-manager.ts
Outdated
| @@ -3,6 +3,7 @@ import { EventEmitter } from 'events'; | |||
| export declare interface SignalManager { | |||
|
|
|||
| fireTooltipSignal(tooltip: { [key: string]: string }): void; | |||
| fireEventClickSignal(payload: {[key: string]: number }): void; | |||
There was a problem hiding this comment.
we should send the keys and value as strings and the receiving side should convert to the number. Later on we'll add other key/value pairs to sync on.
| if (timeStampHeader) { | ||
| const timeStamp = timeStampHeader.getColDef().field; | ||
| const payload = { | ||
| 'timestamp': Number(event.data[timeStamp!]) |
There was a problem hiding this comment.
Send the timestamp as a string
| @@ -141,6 +143,17 @@ export class TimegraphOutputComponent extends AbstractTreeOutputComponent<Timegr | |||
| this.setState({ collapsedNodes: newList }); | |||
| } | |||
|
|
|||
| private selectionChanged(payload: { [key: string]: number }) { | |||
| const offset = this.props.viewRange.getOffset(); | |||
| const rawTimeStamp = payload['timestamp']; | |||
There was a problem hiding this comment.
Here you need to convert it the string to a number. If conversion fails in the code then ignore signal.
Signed-off-by: muddana-satish <[email protected]>
Addressed all the review comments. |
Signed-off-by: muddana-satish [email protected]
depends on eclipse-cdt-cloud/timeline-chart#85