Skip to content

Correct the number entries in the many entries warning dialog #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tuanvtdeptre199 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Correct the number entries in the many entries warning dialog #384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...rc/org/eclipse/tracecompass/tmf/ui/views/ManyEntriesSelectedDialogPreCheckedListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public ManyEntriesSelectedDialogPreCheckedListener(FilteredCheckboxTree filtered
public boolean setSubtreeChecked(Object element, boolean state) {
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;
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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 3

Repository: 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 -100

Repository: 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 10

Repository: 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 -100

Repository: 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 3

Repository: 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.java

Repository: 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.

Copy link
Copy Markdown
Contributor Author

@tuanvtdeptre199 tuanvtdeptre199 Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 24, 2026

Choose a reason for hiding this comment

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

🧩 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:

  1. The user checks a node (e.g. "kernel") in the UI → JFace fires a CheckStateChangedEvent.
  2. FilteredCheckboxTree's listener fires first (registered in super.doCreateTreeViewer()): 
    fCheckboxTreeViewer.addCheckStateListener(event -> {
    if (event.getChecked()) {
        fCheckedObjects.add(event.getElement()); // element added HERE
    }
    ...
});
  3. TriStateFilteredCheckboxTree's listener fires second (registered in its own doCreateTreeViewer()): 
    checkboxTree.addCheckStateListener(event -> setSubtreeChecked(event.getElement(), event.getChecked()));
  4. Inside setSubtreeChecked, getCheckedElements() already contains element at 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.

Copy link
Copy Markdown
Contributor

@jonahgraham jonahgraham Mar 26, 2026

Choose a reason for hiding this comment

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

I concur with @tuanvtdeptre199 (and AI bot eventually).

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 26, 2026

Choose a reason for hiding this comment

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

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!

return showWarning(nb);
}
return false;
Expand Down
Loading