tmf: Refactor virtual table data provider column implementation#318
Conversation
a59b11c to
d0b8401
Compare
| * @return A {@link TmfModelResponse} instance that encapsulates a | ||
| * list of {@link ITableColumnDescriptor} | ||
| */ | ||
| TmfModelResponse<List<ITableColumnDescriptor>> fetchColumns(Map<String, Object> fetchParameters, @Nullable IProgressMonitor monitor); |
There was a problem hiding this comment.
It should return a TmfModelResponse instead whose model will be the List of ITableColumnDescriptors.
Also, there needs to be a default implementation otherwise it's an API breakage
There was a problem hiding this comment.
A default implementation is added that will convert the result from fetchTree().
| @Deprecated | ||
| @Override | ||
| default TmfModelResponse<TmfTreeModel<M>> fetchTree(Map<String, Object> fetchParameters, @Nullable IProgressMonitor monitor) { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
So, there might implementations in custom trace servers that sitll implement fetchTree(). So, for the Trace Compass trace server case, we are going to change the implementation (DataProviderService) to call the new method fetchColumn(). So, a default implementation of fetchColumn below needs to exist that calls fetchTree() and converts the result to the new data structures. Moreover, I think this new deprecated default method should do return super.fetchTree(...).
Alternatively, you keep this interface as is, deprecate it (also deprecate classes that are not needed anymore) and create a completely new interface with the data structures needed. Then in the trace server (DataProviderService) you check for older or new interface and call the corresponding interfaces (also deprecate wrapper classes if applicable). Then, at the next major release you can remove all deprecated classes and remove the call to the older interface in the trace server.
There was a problem hiding this comment.
An old provider implements fetchTree which will override the thrown exception. A new provider will implement fetchColumns and fetchTree will never be called. So the exception would only occur for a new provider that implements neither. Calling super doesn't work in an interface.
| ITmfEventAspect<Long> aspect = TmfBaseAspects.getTimestampNsAspect(); | ||
| long id = fAspectToIdMap.computeIfAbsent(aspect, a -> createColumnId()); | ||
| model.add(new TmfEventTableColumnDataModel(id, -1, Collections.singletonList(aspect.getName()), aspect.getHelpText(), aspect.isHiddenByDefault(), aspect.getDataType())); | ||
| columns.add(new TableColumnDescriptor.Builder().setId(id).setText(aspect.getName()).setTooltip(aspect.getHelpText()).setDataType(aspect.getDataType()).build()); |
There was a problem hiding this comment.
mhh, not sure if we should remove hidden-by-default. We actually never finished to propagate this info further over the TSP to the FE. So, the old code was not finished. But there was a PoC in the FE to do that:
eclipse-cdt-cloud/theia-trace-extension#179
There was a problem hiding this comment.
I've added hidden-by-default to the column descriptor.
| @Override | ||
| public DataType getDataType() { | ||
| return DataType.TIMESTAMP; | ||
| return DataType.NUMBER; |
There was a problem hiding this comment.
I've split this in a separate commit.
Signed-off-by: Patrick Tasse <patrick.tasse@gmail.com>
185f22d to
c435085
Compare
Make virtual table data providers return their columns in a column descriptors list returned by new method fetchColumns(). Add column id to the ITableColumnDescriptor interface. Add hidden by default to the ITableColumnDescriptor interface. Update ITmfVirtualTableDataProvider implementations accordingly, e.g. TmfEventTableDataProvider and SegmentStoreTableDataProvider. Deprecate fetchTree() in ITmfVirtualTableDataProvider. [Added] New method fetchColumns() in ITmfVirtualTableDataProvider [Added] New method getId() in ITableColumnDescriptor [Added] New method isHiddenByDefault() in ITableColumnDescriptor [Deprecated] Deprecate fetchTree() in ITmfVirtualTableDataProvider Signed-off-by: Patrick Tasse <patrick.tasse@gmail.com>
c435085 to
cfed469
Compare
There was a problem hiding this comment.
The Trace Server in incubator repository won't build with this change. Should we we deprecate TmfEventTableColumnData class first?
There was a problem hiding this comment.
This change will require eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator#227 to fix the incubator build.
bhufmann
left a comment
bhufmann
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for clean-up!
2 participants
What it does
Make virtual table data providers return their columns in a column
descriptors list returned by new method fetchColumns().
Add column id to the ITableColumnDescriptor interface.
Add hidden by default to the ITableColumnDescriptor interface.
Update ITmfVirtualTableDataProvider implementations accordingly, e.g.
TmfEventTableDataProvider and SegmentStoreTableDataProvider.
Deprecate fetchTree() in ITmfVirtualTableDataProvider.
How to test
Query the columns of a virtual table data provider and check the returned column descriptors.
Follow-ups
At the next major version, remove fetchTree() and make IVirtualTableDataProvider extend ITmfDataProvider instead of ITmfTreeDataProvider.
Review checklist