Search View: Performance issues on remove and sort

[mehmet-karaman]

• Implemented a batch removal of search matches
• Search matches are removed from their parent, instead of one by one
  which causes performance issues
• It's also fixing the problem, that all elements are removed instead
  only the visible elements (while all others are filtered by the limit
  number)
• Replaces the
  ConcurrentHashMap.newKeySet() with the ConcurrentSkipListSet with the
  comparator and makes the additional sorting obsolete.

Fixes #3735

[iloveeclipse]

@mehmet-karaman : please first create a ticket and explain the problem.
Note: you seem to use wrong git author mail, please use same which you've used in ECA process.

[github-actions]

Test Results

   852 files  ± 0     852 suites  ±0   57m 50s ⏱️ + 1m 2s
 7 848 tests + 4   7 605 ✅ + 5  243 💤 ±0  0 ❌  - 1 
20 070 runs  +12  19 414 ✅ +13  656 💤 ±0  0 ❌  - 1 

Results for commit 526db36. ± Comparison against base commit b819cf9.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

This PR addresses Search View performance and correctness when removing matches, by enabling batch removal at the “enclosing element” (e.g., file/resource) level and ensuring removals aren’t limited to only currently visible (element-limited) results.

Changes:

• Updated AbstractTextSearchViewPage.internalRemoveSelected() to batch-remove matches by selected IFile / IContainer resources before falling back to per-match removal for non-resource selections.
• Added AbstractTextSearchResult.removeElements(...) to remove all matches for selected elements via a single map-entry removal per element.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchViewPage.java	Adds resource-aware pre-processing in “Remove Selected Matches” to remove matches by file/container in a batch.
bundles/org.eclipse.search/newsearch/org/eclipse/search/ui/text/AbstractTextSearchResult.java	Introduces an element-based batch removal method to efficiently drop all matches for selected elements.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[merks]

We're seeing many problems in many builds, including this one.

https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-3680/12/console

[iloveeclipse]

@merks : I've also heard about failing egit nightly builds due slow execution. Looks like something issue affects many different projects.

[merks]

I asked @fredg02 about known problems, but the team is not aware of any general problem, though they are keeping their eyes out.

[iloveeclipse]

I have few comments on tests, beside this looks OK

[iloveeclipse]

@mehmet-karaman : please if you push, resolve all things that are resolved.

[iloveeclipse]

I will push extra commit with test fixes and a TODO that must be implemented

[mehmet-karaman]

Failed test on Linux: org.eclipse.jface.text.tests.reconciler.AbstractReconcilerTest.testReplacingDocumentWhenClean()

Locally it worked on windows.

Searched in github issues and found: #2708

[mehmet-karaman]

@iloveeclips I've replaced the job join thing with a refresh of the viewer, which triggers the content provider.

[iloveeclipse]

The test shows exact the (sporadic) fail I've seen before:
https://github.com/eclipse-platform/eclipse.platform.ui/runs/65745891432

Should have only one direct element ==> expected: <1> but was: <0>
org.opentest4j.AssertionFailedError: Should have only one direct element ==> expected: <1> but was: <0>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:150)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:563)
	at org.eclipse.search.tests.TextSearchResultTest.testBatchRemoveElementsByProject(TextSearchResultTest.java:137)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

[mehmet-karaman]

Started a new trial.. Added this on line 81 to see when we have the results in the view

		while (true) {
			Object[] directElements= getDirectElements();

			if (directElements.length > 0) {
				break;
			}
			System.out.println("Empty");
		}

This snipped helped to get catched in an endless loop and try different things out until the directElements gets entries.

The following snippet after the system.out.println("Empty") helped to get out of the endless loop:

AbstractTextSearchResult searchResult= (AbstractTextSearchResult) fQuery1.getSearchResult();
getSearchPage().getViewer().setInput(searchResult);

This solves our test problem, but I still don't have an answer why refresh() didn't helped and setInput() had to be called.. At that point the Input was equal equal to the searchResult object.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[iloveeclipse]

Thanks Mehmet.

[mehmet-karaman]

Thank you too.