Public API getProviders() in the data provider class#417
Public API getProviders() in the data provider class#417tuanvtdeptre199 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR makes two data provider accessor methods public: ChangesData Provider Accessor Exposure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/AbstractTmfTraceDataProvider.javatmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/model/tree/TmfTreeCompositeDataProvider.java
|
Can you please rebase your change so that your patch can run the CI completely? |
|
Can you please explain in more detail why do you need to access the trace directly to manage multiple traces with one view? |
35d88b0 to
e26e7c0
Compare
|
Hi @arfio, I have rebased my change. |
|
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. |
3ecd0f6 to
cf39c7d
Compare
cf39c7d to
f87b403
Compare
|
@arfio, Thank you for your comment. I understood. If it affects the other extension, then I will revert it. |
|
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. 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? |
|
@arfio , Here is my problem (I'm not sure if I've expressed it clearly, so please forgive me if you're confused.)
|
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
Summary by CodeRabbit