Skip to content

Public API getProviders() in the data provider class#417

Open
tuanvtdeptre199 wants to merge 1 commit into
eclipse-tracecompass:masterfrom
tuanvtdeptre199:publicchildproviderandtrace
Open

Public API getProviders() in the data provider class#417
tuanvtdeptre199 wants to merge 1 commit into
eclipse-tracecompass:masterfrom
tuanvtdeptre199:publicchildproviderandtrace

Conversation

@tuanvtdeptre199

@tuanvtdeptre199 tuanvtdeptre199 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What it does

Recently, I have wanted to improve my 3rd trace views to work with multiple trace packages from the different trace tools. So I want to public the API getProviders() from class TmfTreeCompositeDataProvider.java and getTrace() from class AbstractTmfTraceDataProvider.java

How to test

It does not change any logic code, just public some API, so all views in Trace Compass are expected to work normally.

Follow-ups

Review checklist

  • As an author, I have thoroughly tested my changes and carefully followed the instructions in this template

Summary by CodeRabbit

  • Refactor
    • Expanded the core data provider public API to allow retrieving the underlying trace instance directly.
    • Updated tree composite providers to expose their provider list via a public method while returning an unmodifiable view to help prevent external modification.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR makes two data provider accessor methods public: getTrace() on AbstractTmfTraceDataProvider and getProviders() on TmfTreeCompositeDataProvider; getProviders() now returns an unmodifiable list.

Changes

Data Provider Accessor Exposure

Layer / File(s) Summary
Data provider accessor visibility
tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/AbstractTmfTraceDataProvider.java, tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/tree/TmfTreeCompositeDataProvider.java
getTrace() changed from protected to public; getProviders() changed from protected to public and now returns Collections.unmodifiableList(fProviders).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibble lines and hop through keys,
Two methods blossom from hidden trees,
One reveals the trace I keep,
One guards the list for callers to peep,
A tiny change — the code feels breeze.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions only getProviders() but the PR also modifies getTrace(), making the title incomplete and misleading about the full scope of changes. Update the title to reflect both methods being exposed, such as 'Expose getTrace() and getProviders() as public APIs in data provider classes' to accurately represent all changes in the PR.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tuanvtdeptre199 tuanvtdeptre199 changed the title Public API in the data provider class Public API getTrace() and getProviders() in the data provider class Jun 9, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/tree/TmfTreeCompositeDataProvider.java`:
- Around line 191-193: The public getProviders() currently returns the mutable
backing field fProviders allowing external mutation and bypassing
removeProvider(...) lifecycle/disposal semantics; change getProviders() to
return a defensive/unmodifiable view or copy (e.g., an unmodifiableList or a new
ArrayList copy of fProviders) so callers cannot modify the internal list
directly and must use add/removeProvider(...) to manage provider lifecycle.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4d223a78-2c7d-42e3-a101-6e8a630d25ea

📥 Commits

Reviewing files that changed from the base of the PR and between 75e5dc8 and ee601ea.

📒 Files selected for processing (2)
  • tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/AbstractTmfTraceDataProvider.java
  • tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/tree/TmfTreeCompositeDataProvider.java

@arfio

arfio commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Can you please rebase your change so that your patch can run the CI completely?

@arfio

arfio commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Can you please explain in more detail why do you need to access the trace directly to manage multiple traces with one view?
In my mind, either you need to run an analysis and data provider with an ensemble of trace and you need to use an experiment. Or you have multiple analyses and you need to merge multiple data sources, and you don't need to access the trace only the different dataproviders.

@tuanvtdeptre199 tuanvtdeptre199 force-pushed the publicchildproviderandtrace branch from 35d88b0 to e26e7c0 Compare June 17, 2026 05:17
@tuanvtdeptre199

Copy link
Copy Markdown
Contributor Author

Hi @arfio, I have rebased my change.
Also, for your query, currently I am running an analysis/data provider with an ensemble of traces, and I need to use an experiment.
I think it is better if we can access the trace in the data provider. Because if I or the 3rd side customizes the new TMF trace that extends from the ITmfTrace, they may want to get their trace to do something (For example, as me, I want to get the name of the input trace).
Please help me consider my idea again. If we have a special reason to hide the TMF trace in the data provider from outside, then it is OK for me to use only the child providers.

@arfio

arfio commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

The main reason I am refraining to modify the Data provider API is that it is linked to the TSP. It was done that way so that every analyses that works on Trace Compass standalone would work with the theia trace extension. If you need data about the trace, the data provider would be able to supply it through the already existing API.

@tuanvtdeptre199 tuanvtdeptre199 force-pushed the publicchildproviderandtrace branch 2 times, most recently from 3ecd0f6 to cf39c7d Compare June 18, 2026 02:49
@tuanvtdeptre199 tuanvtdeptre199 force-pushed the publicchildproviderandtrace branch from cf39c7d to f87b403 Compare June 18, 2026 03:06
@tuanvtdeptre199

Copy link
Copy Markdown
Contributor Author

@arfio, Thank you for your comment. I understood. If it affects the other extension, then I will revert it.
But can we publish the API to get the list of providers in the tree composite data provider?

@tuanvtdeptre199 tuanvtdeptre199 changed the title Public API getTrace() and getProviders() in the data provider class Public API getProviders() in the data provider class Jun 18, 2026
@arfio

arfio commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

To be clearer, the data provider class has been put in to separate the analyses side of things which include the traces from the view side of things. This change has been done to be able to create a server which has a dedicated API in this case the TSP (trace server protocol). The trace extension that uses this protocol has been implemented as a plugin both on Theia and Vscode.

The data provider is exposed through the DataProviderService in the incubator trace server implementation.
All of the public getters are implemented by different types of data providers.

If getProviders() were public, then it will not be possible to replicate your view in the extension and as it is made public, other people might use it and it will separate the users in two groups.

I will be more than happy to help you implement the feature that you are trying to make with the current API, and I am sure it will improve Trace Compass. Is it possible for you to explain in more details the issue you are currently facing so that we may help you get it working?

@tuanvtdeptre199

tuanvtdeptre199 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@arfio , Here is my problem (I'm not sure if I've expressed it clearly, so please forgive me if you're confused.)
I created the new trace view -> and I created the Data provider factory, which implements the class IDataProviderFactory -> I override the method public ITmfDataProvider createDataProvider(ITmfTrace trace)

  • In the single trace package, it is simple, I just check if the input ITmfTrace is an instance of my extended trace class -> If yes, I will return my extended data provider class.
  • When deals with experiment trace, that have more than one trace, I choose to return new TmfTreeXYCompositeDataProvider<>(providers, myproviderTittle, myproviderID); which providers is the list of my data provider object instances that extends from ITmfTreeXYDataProvider<@NonNull TmfTreeDataModel>, each provider corresponding to 1 child trace (that is an instance of my extended trace class) in the experiment trace. I think it is reasonable, because the experiment is an esemmble of the traces.
  • It still works ok, but the problem occur when I want to draw more addtional tooltip information into my chart -> I have created my API to get this information in dataprovider class -> Now, in the tooltip provider class/ chart viewer class,I want to get the list of my data provider class that contains in the ojbect TmfTreeXYCompositeDataProvider of the experiments trace. (To more details, at this time the user open the experiments and trace views, the method public void loadTrace(ITmfTrace trace) at chart viewer class is hit -> In this API I can get object TmfTreeXYCompositeDataProvider of the experiment trace)
  • However, as I can not access the list of my provider class that I stored in the object TmfTreeXYCompositeDataProvider before, So I can not add my additional information into the chart view. This is my problem.
  • Of course, I can work around to do it. But I think the best thing is public the API 'getProviders' (getTrace() also, but it still OK if we can not public it).
  • Could you please help me check my problem? Thank you in advance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants