Add fallback in PieChartViewer refresh to not show empty pie chart viewer#413
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pie chart viewer's refresh() now calls newGlobalEntries(this) when fewer than two non-zero event types exist, showing the global pie chart instead of an empty selection. ChangesPie Chart Viewer State Transition
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/internal/tmf/ui/viewers/piecharts/TmfPieChartViewer.java (1)
408-431: ⚡ Quick winConsider avoiding duplicate global chart updates.
When both
refreshGlobalandrefreshSelectionare true and the selection has fewer than two event types,newGlobalEntries(this)is called twice—once at line 410 and again at line 427. This queues redundant async updates to the global chart with identical data.⚡ Proposed optimization
if (refreshGlobal) { /* will update the global pc */ getCurrentState().newGlobalEntries(this); } if (refreshSelection) { // Check if the selection is empty int nbEventsType = 0; Map<String, Long> selectionModel = getTotalEventCountForChart(false); for (Long l : selectionModel.values()) { if (l != 0) { nbEventsType++; } } // Check if the selection is empty or if // there is enough event types to show in the piecharts // if there aren't enough, show the global pie chart if (nbEventsType < 2) { - getCurrentState().newGlobalEntries(this); + if (!refreshGlobal) { + getCurrentState().newGlobalEntries(this); + } } else { getCurrentState().newSelection(this); } }🤖 Prompt for 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. In `@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/internal/tmf/ui/viewers/piecharts/TmfPieChartViewer.java` around lines 408 - 431, The code in TmfPieChartViewer calls getCurrentState().newGlobalEntries(this) twice when both refreshGlobal and refreshSelection are true and the selection has fewer than two event types; modify the logic so newGlobalEntries(this) is only queued once by computing the selection branch first (using getTotalEventCountForChart() and nbEventsType) to decide between newSelection(this) or newGlobalEntries(this), and then only call newGlobalEntries(this) for the refreshGlobal path if the global update hasn't already been queued (i.e., track a local boolean like globalQueued or check refreshSelection outcome); reference methods: getTotalEventCountForChart(), getCurrentState().newGlobalEntries(), getCurrentState().newSelection(), and the refreshGlobal/refreshSelection flags.
🤖 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.
Nitpick comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/internal/tmf/ui/viewers/piecharts/TmfPieChartViewer.java`:
- Around line 408-431: The code in TmfPieChartViewer calls
getCurrentState().newGlobalEntries(this) twice when both refreshGlobal and
refreshSelection are true and the selection has fewer than two event types;
modify the logic so newGlobalEntries(this) is only queued once by computing the
selection branch first (using getTotalEventCountForChart() and nbEventsType) to
decide between newSelection(this) or newGlobalEntries(this), and then only call
newGlobalEntries(this) for the refreshGlobal path if the global update hasn't
already been queued (i.e., track a local boolean like globalQueued or check
refreshSelection outcome); reference methods: getTotalEventCountForChart(),
getCurrentState().newGlobalEntries(), getCurrentState().newSelection(), and the
refreshGlobal/refreshSelection flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78535148-aeb1-4e40-b9cc-30b293b40b70
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/internal/tmf/ui/viewers/piecharts/TmfPieChartViewer.java
MatthewKhouzam
left a comment
MatthewKhouzam
left a comment
There was a problem hiding this comment.
Looks good to me!
|
Just a friendly reminder, if you used AI in any patch (I doubt this one has AI), you need to bring it up in the commit message. Like "This patch was created with the assistance of chatgpt 5.5" |
There seems to be a CI check fail that matches the CI failure on main, is this normal? Also, would it be possible to include this change in some form of nightly build for trace compass 11.3 since I am working on a project in Eclipse 2026-03? Thanks |
|
Looks like this work is fine, but it fails because of the upgrade. rebase it and it should work. |
Sorry but I might be confused. Which commit do I rebase on top of? My fork is built on top of the current latest commit on main already. |
|
The problem is that the "OK" button got replaced with an "APPLY" button in the tests. not your code, it's the platform that's changing. So that's why I'm saying a rebase would fix it. @arfio could you confirm? |
|
The release is today, after that the rebase will fix the issue. For now the build is not using the new platform by default. |
Just making sure, if I want to use an official eclipse download repo link, this PR's changes will only be reflected in the tracecompass 12.0 links? There are no possibilities to have the PR change on an 11.3 version of tracecompass without me using my local version of tracecompass directly? Thanks! |
… but not enough event types in the selection, show the global pie chart instead.
GuillaumeZhang321
force-pushed
the
pie_chart_fallback
branch
from
a8923ff to
a173242
Compare
|
Hello, Would it be possible to restart the workflow check for this PR and #416 ? Thanks! |
3 participants
… but not enough event types in the selection, show the global pie chart instead.
What it does
In
org.eclipse.tracecompass.internal.tmf.ui.viewers.piecharts.TmfPieChartViewer.refresh, instead of making an empty viewer when less than two events types are selected, show the change shows the global pie chart.How to test
In statistics view, select only one event type and see that instead of "No Data" being shown, the global pie chart is left.
Follow-ups
Addresses #412
Review checklist
Summary by CodeRabbit
Release Notes