-
Notifications
You must be signed in to change notification settings - Fork 28
Add generic XY view #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generic XY view #348
Conversation
2ff9cd1 to
bb02fa2
Compare
bb02fa2 to
6b9cba7
Compare
c28017c to
edf4f4f
Compare
edf4f4f to
a6f06b2
Compare
|
The build will require the changes from eclipse-cdt-cloud/tsp-typescript-client#138 to succeed. |
a6f06b2 to
f0e38f0
Compare
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing the generic xy widget and also to extracting common code in a shared utility class. I did a first review and some testing. I was able to see the Function Density view with Trace Compass server. I also tried other chart types and the time axis as described in the description.
I added some review comments and findings. Please have a look
| } | ||
|
|
||
| function buildColumns(headers?: HeaderSpec[]): ColumnSpec[] { | ||
| if (headers && headers.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (headers?.length)
| { | ||
| display: false, | ||
| gridLines: { display: false, drawBorder: false, drawTicks: false }, | ||
| ticks: yTickFix as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here a suggestion for another, separate PR. I think it would be too much for this PR.
So, for the Function Duration Density chart of the Trace Compass server, I noticed very high and low numbers, so that in a linear scale the low numbers are not visible. For those cases logarithmic scale would be better. Since the viewer doesn't allow for users to change UI settings and modify the scale, I wonder if we should set it y-scale for bar charts either by default to logarithmic or if the difference between lowest and highest number exceeds a certain threshold. When using logarithmic scale we should probably enable the horizontal gridlines so that it's obvious for the user. Since the y-axis is our custom implementation, then it needs to be updated as well for logarithmic scale. Even better would be to support view customizations in FE.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be a nice improvement. But I fear that treating bar chart as a special citizen would over-complicate the code. What about using logarithmic y-scale for all xy chart types (line/scatter/bar) if the difference between lowest and highest number exceeds a certain threshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a good start. Do this in a separate PR maybe.
| export function computeYRange( | ||
| datasets: DatasetLike[] | undefined, | ||
| isScatter = false, | ||
| pad = 0.01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this padding for? Ok found it, It was already in the original code.
| } | ||
|
|
||
| export class ColorAllocator { | ||
| private map = new Map<string, number>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be readonly
| isScatter = false, | ||
| pad = 0.01 | ||
| ): { min: number; max: number } { | ||
| if (!datasets || !datasets.length) return { min: 0, max: 1 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!datasets?.length)
| const newStartFloat = Number(this.mousePanningStart) - xNow / this.resolution; | ||
|
|
||
| const min = BigInt(0); | ||
| const max = this.props.unitController.absoluteRange - this.props.unitController.viewRangeLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unitcontroller is for the common time axis
| if (ev.shiftKey && !ev.ctrlKey && this.props.unitController.selectionRange) { | ||
| this.isSelecting = true; | ||
| this.setState({ cursor: 'crosshair' }); | ||
| this.props.unitController.selectionRange = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unitcontroller is for the common time axis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the selectionRange feature.
|
|
||
| private updateViewRange(start: bigint, end: bigint): void { | ||
| const [s, e] = start < end ? [start, end] : [end, start]; | ||
| this.props.unitController.viewRange = { start: s, end: e }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unitcontroller is for the common time axis. It can't be updated from this class unless x is the time axis
| } | ||
|
|
||
| private zoom(isZoomIn: boolean): void { | ||
| if (this.props.unitController.viewRangeLength >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: unitcontroller is for the common time axis
| private pan(left: boolean): void { | ||
| const vr = this.props.unitController.viewRange; | ||
| const abs = this.props.unitController.absoluteRange; | ||
| this.props.unitController.viewRange = panRange(vr, abs, left); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here: unitcontroller is for the common time axis
f0e38f0 to
4dbc258
Compare
siwei-zhang-work
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I've applied the comments.
| return getCollapsedNodesFromAutoExpandLevel(listToTree(entries, columns as any), autoExpandLevel); | ||
| } | ||
|
|
||
| export type DefaultCheckMode = 'strict-default-flag' | 'leaf-if-has-data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that adding this flag over-complicate things. I removed it and kept the behavior the same.
| return { min: 0, max: 1 }; | ||
| } | ||
| const range = localMax - localMin; | ||
| return { min: localMin - range * pad, max: localMax + range * pad }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to keep it the same, changed it back.
| { | ||
| display: false, | ||
| gridLines: { display: false, drawBorder: false, drawTicks: false }, | ||
| ticks: yTickFix as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be a nice improvement. But I fear that treating bar chart as a special citizen would over-complicate the code. What about using logarithmic y-scale for all xy chart types (line/scatter/bar) if the difference between lowest and highest number exceeds a certain threshold?
| } | ||
|
|
||
| private computeDesiredSampleNb(): number { | ||
| // TODO: Currently the number of samples is calculated only for bars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because the bars of each series are drawn beside each other, right?
Yes, this is needed because the bars of each series are drawn beside each other so we can't keep the original calculation of sample numbers.
So, we would have to add options to the tree model, that are specific for xy, timegraph, data tree etc.
I don't fully understand what you mean here. I was thinking to add the display type in XyEntry?
| } | ||
|
|
||
| private endSelection = (_e: MouseEvent): void => { | ||
| if (this.clickedMouseButton === 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature was what I wanted to achieve in the initial commit in the server by enabling the xValues in request parameters. It requires adding XYValuesDescription to the tree model response to let server reply with available xValues ranges. However, all the /tree endpoint in the server currently is using the same function. So it'll require quite a big change in the APIs. I've now disabled the zoom for non-time x-axis.
| const rawLabel = labels[index]; | ||
|
|
||
| if (this.isTimeAxis) { | ||
| // If it's a time axis, the label is a bigint timestamp. Format it as seconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is always timestamp if this.isTimeAxis is true. Modified to do this transformation while creating the labels.
| if (ev.shiftKey && !ev.ctrlKey && this.props.unitController.selectionRange) { | ||
| this.isSelecting = true; | ||
| this.setState({ cursor: 'crosshair' }); | ||
| this.props.unitController.selectionRange = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the selectionRange feature.
That's pretty much what I mean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a second review I have only some minor comments. The functionality looks good for this PR and my previous comment are addressed:
I'll release the latest tsp-typescript-client which you can use in this PR. I'll let you know when it's donetsp-typescript-client v0.8.0 is released- I noticed that the progress wheel in the view is not shown when analysis is running (message in the view)
- Please rebase to latest master (there were some changes brought in from the traceviewer-libs, i.e. mainly linter related updates.
@PatrickTasse could you please have look into this PR as well? Thanks
4dbc258 to
bb157d8
Compare
|
Thanks for the review. Rebased on master and added the progress wheel that I accidentally deleted. |
Extract util methods from abstract-xy-output-component so that later they can be shared with generic xy view. Signed-off-by: Siwei Zhang <[email protected]>
bb157d8 to
5d67d97
Compare
| ], | ||
| "dependencies": { | ||
| "tsp-typescript-client": "^0.7.0" | ||
| "tsp-typescript-client": "^0.8.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the version has to be updated in the following files as well:
- local-libs/traceviewer-libs/react-components/package.json
- vscode-trace-common/package.json
Implement the new generic xy view to support non-time x-axis xy view. This new view uses a new endpoint in the tsp introduced in: eclipse-cdt-cloud/trace-server-protocol#114 Signed-off-by: Siwei Zhang <[email protected]>
5d67d97 to
f690042
Compare
bhufmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you very much for the contribution It enhances the application to be able to provide density views. We are much closer to feature parity to Eclipse Trace Compass.
What it does
Adds the generic XY view as introduced in eclipse-cdt-cloud/trace-server-protocol#114.
Note on Selection Display:
The initial design intended to show selection data in the bar chart (similar to the data tree view). This was revised during implementation because the legacy XY endpoint does not show selection, and adding extra bars for selection would be visually confusing with a time-based x-axis. Consequently, selection is no longer displayed in this view.
Dependencies
This change requires the updates from eclipse-cdt-cloud/tsp-typescript-client#138.
Related Merged Pull Requests
The corresponding server and protocol updates are already merged in:
How to test
The non-time x-axis with bar chart can be tested with a lttng-ust trace using function density view.
For other axis and type there's no existing view for now, but can be tested by doing small modifications in the code:
Testing other view types
AbstractTreeGenericXYCommonXDataProvider#getDisplayType()can be modified to returnDisplayType.LINE/SCATTER.Testing time-based x-axis
The following patch can be addressed in
org.eclipse.tracecompassfor a temporary test.Follow-ups
No follow-ups.
Review checklist