Add fallback in PieChartViewer refresh to not show empty pie chart viewer#413
Conversation
… but not enough event types in the selection, show the global pie chart instead.
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 📝 WalkthroughWalkthroughThe pie chart viewer's 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" |
2 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