Align code with the TSP specification #147
Conversation
|
This uses the following PR in Trace Server Protocol: eclipse-cdt-cloud/trace-server-protocol#37 Requires the Trace Server patches: https://git.eclipse.org/r/c/tracecompass.incubator/org.eclipse.tracecompass.incubator/+/170409 and previous And needs the TSP-typescript-client PR: eclipse-cdt-cloud/tsp-typescript-client#4 Also, for the XY columns, since no data provider implements it correctly yet on the server side, there's no dp that will have columns (as of today october 8th 2020) |
|
Also, this chain of PRs has focused on the data models returned for the various outputs. There should be another round of patches for query parameters (I did some for the table queries though) and the traces/experiment endpoints. |
| columnEntries.forEach(entry => { | ||
| const id = this.showIndexColumn ? ++entry.id : entry.id; | ||
| columnEntries.forEach(columnHeader => { | ||
| const id = this.showIndexColumn ? ++columnHeader.id : columnHeader.id; |
There was a problem hiding this comment.
later patch... can we split this... I really hate counting on pre/post increments.
| }, | ||
| data: { | ||
| stateValue: -1 | ||
|
|
There was a problem hiding this comment.
could you comment on why a null state isn't -1?
There was a problem hiding this comment.
the concept of stateValue is now gone from the TSP!!...
though as I did this patch, I'm ambivalent... stateValue was small number, the style is more verbose. But having both style and value was confusing, as it's either one or the other. And style offers more possibility.
For now in the Trace Server, when the style is not set in the state, it's using the value's string value as a style... And if it's a null state, then there's no style at all.
There was a problem hiding this comment.
I think we could fix the style vs value using mixed type in the protocol, as described here: https://swagger.io/docs/specification/data-models/data-types/
| return { | ||
| color: parseInt(elementStyle.color, 16), | ||
| height: this.props.style.rowHeight * elementStyle.height, | ||
| color: 0xCACACA, |
There was a problem hiding this comment.
Ok, so we check if there's no output style instead?
Also... should we extract all the colors as constants (separate patch)
There was a problem hiding this comment.
One of my next task will be #35
I'll look into the default values and everything then. I just made it work for now.
|
I just saw the approval here, thanks! So missing is only the incubator's trace server patches. |
tahini
force-pushed
the
align_tsp
branch
2 times, most recently
from
6d10403 to
1e81f15
Compare
|
@MatthewKhouzam are you ok with the change? The dependent patches (tsp-typescript-client and trace server) are merged. |
The trace server protocol and the tsp-typescript-client have been updated. This makes the trace extension match the newer versions. It updates the filter-tree (left part of xy and timegraphs) to use labels and column headers instead of extra keys. Signed-off-by: Geneviève Bastien <[email protected]>
|
Merging this now, don't forget to yarn download:server again! |
ghost
mentioned this pull request
The trace server protocol and the tsp-typescript-client have been
updated. This makes the trace extension match the newer versions.
It updates the filter-tree (left part of xy and timegraphs) to use
labels and column headers instead of extra keys.
Signed-off-by: Geneviève Bastien [email protected]