fix: SelectAllAction of SearchView correctly propagates all selected items

[vlakn]

Description

When using CTRL+A in the Search View to select the entire content of the view, all items are visually selected. However, not all selected elements are propagated to subsequent actions. Instead, only the elements that were selected before pressing CTRL+A are passed on.

Expected Behavior

All items that are visually selected after pressing CTRL+A should be propagated to subsequent actions.

How it was fixed

The SelectAllAction implementation in the org.eclipse.search plugin was updated to ensure that after visually selecting all items (e.g., via CTRL+A), the selection is also properly propagated to subsequent actions. This was achieved by explicitly setting the viewer's selection which triggers a SelectionChangedEvent after updating the widget selection, mirroring the approach used in the SelectAllAction classes of org.eclipse.debug.ui and org.eclipse.jdt.ui.

Steps to Reproduce

• Open the Search View. Ensure the Search View contains entries.
• Ensure that some items are selected.
• Press CTRL+A to select all items in the view.
• Perform an action that operates on the selection (e.g., use the handler in this patch)
• Observe that only the items that were selected before pressing CTRL+A are affected, not all visually selected items.

Before

After

[vogella]

Thank you for the fix.

The fix itself looks good. Tree#setSelection(...) is SWT-level and does not fire a JFace SelectionChangedEvent. The TableViewer branch right next to it already worked around this with fViewer.setSelection(fViewer.getSelection()), so applying the same idiom to the TreeViewer branch keeps both branches consistent (the pattern is not only in debug.ui / jdt.ui, but already in the sibling branch one line down).

A few small things I would ask you to clean up before merge:

1. Unrelated whitespace churn in SelectAllAction.java (lines 47, 68, 69). The fViewer= viewer; style (no space before =) is the convention used throughout this class and most of the org.eclipse.search bundle. Please revert those so the diff stays focused on the behavioral fix.

2. Test assertion style in SelectAllActionTest.java:

   • assertTrue(event[0] != null, ...) should be assertNotNull(event[0], ...).
   • assertTrue(sel.size() == 3, ...) should be assertEquals(3, sel.size(), ...), matching the assertEquals(3, selection.size(), ...) lines already present in the same methods.

3. Copy/paste leftover: testSelectAllWithEmptyTreeViewer uses the message "Selection in event should be empty for an empty table" on a tree test.

4. testSelectAllWithTreeViewerOnlySelectsExpandedItems final assertion: ((IStructuredSelection) event[0].getSelection()).size() > 1 with message "SelectionChangedEvent should have been fired" is too loose. It should assert size() == 4 (matching the assertEquals(4, expandedSelection.size(), ...) right above), and the message should reflect the size check.

5. \$NON-NLS-1\$ markers: present on "parent", "child1", "sibling" but missing on "node1", "node2", "node3" in testSelectAllWithTreeViewerFlatList. Please make it consistent.

[vogella]

Alternatively to 1, you can also create a PR with only this formatting change and rebase this one on top of it.

[vlakn]

Thank you for the thorough review and your feedback.
I've implemented your suggestions and cleaned up the remaining points you mentioned.

[github-actions]

Test Results

   852 files  ± 0     852 suites  ±0   53m 2s ⏱️ - 2m 7s
 7 920 tests + 6   7 677 ✅ + 6  243 💤 ±0  0 ❌ ±0 
20 262 runs  +18  19 607 ✅ +18  655 💤 ±0  0 ❌ ±0 

Results for commit 570b1d8. ± Comparison against base commit afacf90.

♻️ This comment has been updated with latest results.

[vogella]

How can I reproduce the error in the IDE?

If I search for something, like "test" and press CTRL+A, CTRL+C everything is already selected. Please describe the situation in which this does not work for you. (Might be a platform bug)

[merks]

Did you test that the paste of the clipboard content contains everything visually selected?

[vlakn]

We encountered this issue on our side because we have implemented custom actions for certain refactorings. While working on these actions, we noticed the misbehavior that refactorings were not executed on all selected plugins.
This can be reproduced, for example, using the reproducer included in the attached patch mentioned in the PR description.

However, the incorrect behavior also causes issues directly in the IDE. One concrete example I observed is reproducable in Java Search:
When performing a Java search, some artifacts (e.g. packages, classes, methods...) offer the action to search for references across the workspace.

If all items in the Search View are selected using Ctrl+A, this selection can include items for which this action should not be available at all. In the faulty case, the check that determines whether an action should be shown in the context menu incorrectly succeeds.
As a result, the action is displayed even though it is not valid for the full selection. Executing the action then leads to the observed error message.

This issue is not limited to the “Search for References” action. Similar problems can be observed with other selection-based actions as well. With the changes in this PR, these actions are now correctly filtered and are no longer offered for invalid selections and thereby preventing the error altogether.

[vogella]

@vlakn code looks good to me but I will not commit if I cannot reproduce in plain Eclipse (without extra plug-ins). Sorry, to many things on my side to get into client code in my free time.

Lets hope other committers like @merks can help here.

[merks]

@vlakn

I tested this with you helpful handler and it works well with your fix (and poorly without your fix).

Could you please squash this to a single commit. I'll merge it after that...

[merks]

Thank you for your contribution and patience.