Correct the number entries in the many entries warning dialog#384
Correct the number entries in the many entries warning dialog#384tuanvtdeptre199 wants to merge 1 commit intoeclipse-tracecompass:masterfrom
Conversation
📝 WalkthroughWalkthroughA single numeric adjustment reduces the subtree size calculation by 1 in the warning threshold logic of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.java`:
- Line 51: The computation for nb in ManyEntriesSelectedDialogPreCheckedListener
is undercounting because it subtracts 1 before the subtree is actually checked;
update the logic that calculates nb (currently using
fFilteredCheckboxTree.getCheckedElements().length +
getSubTreeSize(contentProvider, element) - 1) to remove the "- 1" offset so it
becomes fFilteredCheckboxTree.getCheckedElements().length +
getSubTreeSize(contentProvider, element); alternatively, compute the checked
count after invoking checkSubtree() or otherwise ensure the post-mutation total
(including the element itself) is used when evaluating the nb > 20 boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8804f64-6ec5-44ec-82ae-d5f4a46e8dea
📒 Files selected for processing (1)
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.java
| if (state) { | ||
| ITreeContentProvider contentProvider = (ITreeContentProvider) fFilteredCheckboxTree.getCheckboxTreeViewer().getContentProvider(); | ||
| int nb = fFilteredCheckboxTree.getCheckedElements().length + getSubTreeSize(contentProvider, element); | ||
| int nb = fFilteredCheckboxTree.getCheckedElements().length + getSubTreeSize(contentProvider, element) - 1; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate listener interface and implementation =="
fd 'IPreCheckStateListener.java'
fd 'ManyEntriesSelectedDialogPreCheckedListener.java'
fd 'FilteredCheckboxTree.java'
echo
echo "== Inspect interface contract =="
rg -n -C4 --type=java 'interface IPreCheckStateListener|boolean setSubtreeChecked\('
echo
echo "== Inspect call sites and ordering in FilteredCheckboxTree =="
rg -n -C12 --type=java 'IPreCheckStateListener|setSubtreeChecked\(|getCheckedElements\(|setChecked|setSubtreeChecked'
echo
echo "== Inspect warning threshold usage =="
rg -n -C4 --type=java 'showWarning\(|nb > 20|getSubTreeSize\('Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 50402
🏁 Script executed:
fd 'FilteredCheckboxTree.java' -x cat -n {} \; | grep -A 20 'checkSubtree'Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4964
🏁 Script executed:
rg -n 'IPreCheckStateListener|setPreCheckStateListener|fListener|listener\.' tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/FilteredCheckboxTree.java -A 3 -B 3Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 71
🏁 Script executed:
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/FilteredCheckboxTree.java | head -250 | tail -100Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 3834
🏁 Script executed:
rg -n 'setSubtreeChecked\|IPreCheckStateListener' tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java -B 5 -A 10Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 71
🏁 Script executed:
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java | head -200 | tail -100Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 5285
🏁 Script executed:
rg -n 'listener.*setSubtreeChecked|fListener|IPreCheckStateListener' tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/ -B 3 -A 3Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4812
🏁 Script executed:
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java | sed -n '80,100p'Repository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 1118
🏁 Script executed:
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.javaRepository: eclipse-tracecompass/org.eclipse.tracecompass
Length of output: 4467
Line 51 undercounts by 1 due to pre-mutation callback timing.
The listener is invoked before checkSubtree() mutates the checked state. When the formula computes getCheckedElements().length + getSubTreeSize(element) - 1, it reads the pre-mutation count plus the subtree size (which includes the element itself), then subtracts 1—resulting in pre_count + descendants instead of the correct post-mutation total of pre_count + 1 + descendants. This causes the boundary case (e.g., 20 checked + 1 leaf = 21 total) to be undercounted as 20, suppressing the warning when nb > 20 should trigger. Remove the - 1 offset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.java`
at line 51, The computation for nb in
ManyEntriesSelectedDialogPreCheckedListener is undercounting because it
subtracts 1 before the subtree is actually checked; update the logic that
calculates nb (currently using fFilteredCheckboxTree.getCheckedElements().length
+ getSubTreeSize(contentProvider, element) - 1) to remove the "- 1" offset so it
becomes fFilteredCheckboxTree.getCheckedElements().length +
getSubTreeSize(contentProvider, element); alternatively, compute the checked
count after invoking checkSubtree() or otherwise ensure the post-mutation total
(including the element itself) is used when evaluating the nb > 20 boundary.
There was a problem hiding this comment.
For example, in the "CPU Usage" view, when the user selects the parent node in the tree table ("kernel" node), the method FilteredCheckboxTree.doCreateTreeViewer(Composite parentComposite, int style) will automatically hit and add the selected parent node into the field fCheckedObjects -> After that, the method ManyEntriesSelectedDialogPreCheckedListener.setSubtreeChecked(Object element, boolean state) is called, and at this moment, fFilteredCheckboxTree.getCheckedElements().length has already counted the element before, so we have to -1 to get the correct number selected entry before the user's action in the tree table.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the pre-checking behaviour described by the user
rg -n 'fCheckedObjects|checkSubtree\|setSubtreeChecked\|setChecked\|IPreCheckStateListener' \
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/FilteredCheckboxTree.java \
-B 3 -A 6
echo "---"
rg -n 'checkSubtree\|setSubtreeChecked\|fListener\.' \
tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java \
-B 3 -A 8
Length of output: 2541
🏁 Script executed:
#!/bin/bash
# Look at how setSubtreeChecked is called in TriStateFilteredCheckboxTree
cat -n tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/widgets/timegraph/dialogs/TriStateFilteredCheckboxTree.java | sed -n '1,250p'
Length of output: 11885
@tuanvtdeptre199, you are absolutely right, and I apologize for the incorrect review comment. After tracing the actual execution order in the code, your explanation is fully confirmed:
Confirmed event flow:
- The user checks a node (e.g. "kernel") in the UI → JFace fires a
CheckStateChangedEvent. FilteredCheckboxTree's listener fires first (registered insuper.doCreateTreeViewer()):fCheckboxTreeViewer.addCheckStateListener(event -> { if (event.getChecked()) { fCheckedObjects.add(event.getElement()); // element added HERE } ... });
TriStateFilteredCheckboxTree's listener fires second (registered in its owndoCreateTreeViewer()):checkboxTree.addCheckStateListener(event -> setSubtreeChecked(event.getElement(), event.getChecked()));
- Inside
setSubtreeChecked,getCheckedElements()already containselementat this point.
This is also confirmed by the snapshot logic at the top of setSubtreeChecked (lines 82–87), which explicitly calls prevChecked.remove(element) when state=true — precisely because element is already in fCheckedObjects and needs to be excluded from the "before" snapshot for a potential revert.
So the arithmetic is:
getCheckedElements().length= pre-action count +1 (element already included)getSubTreeSize(element)= 1 (element itself) + descendants- Without
- 1: double-counts the element → off by 1 - With
- 1:(pre_count + 1) + (1 + descendants) - 1=pre_count + 1 + descendants✓
The fix is correct as written.
✏️ Learnings added
Learnt from: tuanvtdeptre199
Repo: eclipse-tracecompass/org.eclipse.tracecompass PR: 384
File: tmf/org.eclipse.tracecompass.tmf.ui/src/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.java:51-51
Timestamp: 2026-03-24T04:41:31.693Z
Learning: In `ManyEntriesSelectedDialogPreCheckedListener.setSubtreeChecked()` (Java, Eclipse Trace Compass TMF UI), the `- 1` in the formula `fFilteredCheckboxTree.getCheckedElements().length + getSubTreeSize(contentProvider, element) - 1` is intentional and correct. When the user checks a node, `FilteredCheckboxTree`'s own `CheckStateListener` (registered in `doCreateTreeViewer()`) adds the element to `fCheckedObjects` before `TriStateFilteredCheckboxTree`'s listener calls `setSubtreeChecked()`. So `getCheckedElements()` already includes the element at the time `setSubtreeChecked` is invoked, and `getSubTreeSize()` also counts the element (returns 1 + descendants). Without `- 1` the element would be double-counted. This is also confirmed by the `prevChecked.remove(element)` snapshot correction in `TriStateFilteredCheckboxTree.setSubtreeChecked()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I concur with @tuanvtdeptre199 (and AI bot eventually).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Hi @jonahgraham -san, could you please help me check this change? Thank you. |
jonahgraham
left a comment
jonahgraham
left a comment
There was a problem hiding this comment.
I tested this with "System Call Latency vs Time" on the cat-kernel example extracted from the tracevizlab tar. I used this one instead of CPU usage because this view has multiple levels of hierarchy.
@MatthewKhouzam over to you to review
| if (state) { | ||
| ITreeContentProvider contentProvider = (ITreeContentProvider) fFilteredCheckboxTree.getCheckboxTreeViewer().getContentProvider(); | ||
| int nb = fFilteredCheckboxTree.getCheckedElements().length + getSubTreeSize(contentProvider, element); | ||
| int nb = fFilteredCheckboxTree.getCheckedElements().length + getSubTreeSize(contentProvider, element) - 1; |
There was a problem hiding this comment.
I concur with @tuanvtdeptre199 (and AI bot eventually).
2 participants
What it does
Correct the number entry in the message in the "Many entries selected" dialog:
+) In method
setSubtreeChecked(Object element, boolean state),fFilteredCheckboxTree.getCheckedElements().lengthcounts theelementas one entry, but the methodgetSubTreeSize(contentProvider, element)also counts it again, which makes the number of entries in the warning message incorrect.+) For example: in the "CPU Usage" view, if the tree table has one parent node "kernel" and 39 child processes in this node. If the user selects the "kernel" node -> the "Many entries selected" dialog opens, and the warning message is "You have selected 41 entries....". But the correct message should be "You have selected 40 entries ......"
-> We have to -1 to get the correct number of entries in the method
setSubtreeChecked(Object element, boolean state)How to test
+) Test through the "CPU Usage" view, when the "Many entries selected" dialog opens, the warning messages show the correct number of entries that will be selected in the tree table (parent node + child node)
Follow-ups
Review checklist