Text control should not redraw indefinitely

In some scenarios, `Text` control redraws on every event loop iteration.
See eclipse-platform/eclipse.platform.ui#3920

Replace native text query in search-field paint with Java-side cache

Introduce a searchFieldHasText boolean on Text, refreshed at every
SWT.Modify emission point (append, cut, insert, paste/replace,
setText, setTextChars, verify-listener replacement, and
textDidChange). The helper only queries stringValue when the display
is not currently painting, so the Java-side read cannot re-arm the
loop. The paint method in drawInteriorWithFrame_inView_searchfield
consults the cache instead of calling stringValue on the cell.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Lars Vogel <Lars.Vogel@vogella.com>

Author: 3 people

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java

@@ -70,6 +70,18 @@ public class Text extends Scrollable {
 	long actionSearch, actionCancel;
 	APPEARANCE lastAppAppearance;
 
+	/*
+	 * Cached emptiness of the text for the SWT.SEARCH | SWT.ICON_CANCEL paint
+	 * path only. AppKit's NSSearchFieldCell stringValue selector posts
+	 * setNeedsDisplay: synchronously when invoked during a draw pass, turning
+	 * drawInteriorWithFrame_inView_searchfield's cancel-icon visibility check
+	 * into an infinite redraw loop. This flag is updated at the text-mutation
+	 * entry points (outside any paint), so the paint method never has to query
+	 * native state.
+	 * See https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+	 */
+	boolean searchFieldHasText;
+
 	/**
 	* The maximum number of characters that can be entered
 	* into a text widget.
@@ -331,6 +343,7 @@ public void append (String string) {
 		widget.scrollRangeToVisible (range);
 		widget.setSelectedRange(range);
 	}
+	updateSearchFieldHasText();
 	if (string.length () != 0) sendEvent (SWT.Modify);
 }
 
@@ -639,6 +652,7 @@ public void cut () {
 		}
 	}
 	Point newSelection = getSelection ();
+	updateSearchFieldHasText();
 	if (!cut || !oldSelection.equals (newSelection)) sendEvent (SWT.Modify);
 }
 
@@ -745,11 +759,22 @@ void drawInteriorWithFrame_inView_searchfield (long id, long sel, NSRect cellFra
 	SWTSearchFieldCell cell = new SWTSearchFieldCell(_cell.id);
 
 	if (cell.searchButtonCell() != null) {
-		cell.searchButtonCell().drawInteriorWithFrame(cell.searchButtonRectForBounds(cellFrame), view);
+		NSRect searchRect = cell.searchButtonRectForBounds(cellFrame);
+		cell.searchButtonCell().drawInteriorWithFrame(searchRect, view);
 	}
 
-	if (cell.cancelButtonCell() != null && ((NSSearchField) view).stringValue().length() > 0) {
-		cell.cancelButtonCell().drawInteriorWithFrame(cell.cancelButtonRectForBounds(cellFrame), view);
+	NSCell cancelCell = cell.cancelButtonCell();
+	/*
+	 * FIX: Use the Java-side searchFieldHasText cache rather than calling
+	 * stringValue on the search field during paint. Both NSControl.stringValue
+	 * and NSCell.stringValue dispatch to the same AppKit selector, which
+	 * posts setNeedsDisplay: as a side effect and re-arms the paint loop we
+	 * are trying to complete. See eclipse.platform.ui#3920 and the
+	 * updateSearchFieldHasText() call sites for how the cache is kept current.
+	 */
+	if (cancelCell != null && searchFieldHasText) {
+		NSRect cancelRect = cell.cancelButtonRectForBounds(cellFrame);
+		cancelCell.drawInteriorWithFrame(cancelRect, view);
 	}
 }
 
@@ -1433,6 +1458,7 @@ public void insert (String string) {
 		}
 		widget.textStorage ().replaceCharactersInRange (range, str);
 	}
+	updateSearchFieldHasText();
 	if (string.length () != 0) sendEvent (SWT.Modify);
 }
 
@@ -1571,6 +1597,7 @@ void _paste (boolean enableUndo) {
 			}
 		}
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2247,6 +2274,7 @@ public void setText (String string) {
 		widget.setString (str);
 		widget.setSelectedRange(new NSRange());
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2300,6 +2328,7 @@ public void setTextChars (char[] text) {
 		widget.setString (str);
 		widget.setSelectedRange(new NSRange());
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2397,7 +2426,10 @@ boolean shouldChangeTextInRange_replacementString(long id, long sel, long affect
 			result = false;
 		}
 	}
-	if (!result) sendEvent (SWT.Modify);
+	if (!result) {
+		updateSearchFieldHasText();
+		sendEvent (SWT.Modify);
+	}
 	return result;
 }
 
@@ -2434,9 +2466,23 @@ void textViewDidChangeSelection(long id, long sel, long aNotification) {
 @Override
 void textDidChange (long id, long sel, long aNotification) {
 	if ((style & SWT.SINGLE) != 0) super.textDidChange (id, sel, aNotification);
+	updateSearchFieldHasText();
 	postEvent (SWT.Modify);
 }
 
+/*
+ * Refresh searchFieldHasText from the native control. Only safe to call
+ * outside a paint pass; querying stringValue during paint re-arms the
+ * very loop this cache exists to break (see eclipse.platform.ui#3920).
+ */
+void updateSearchFieldHasText() {
+	if ((style & SWT.SEARCH) == 0) return;
+	if (display == null) return;
+	if (display.isPainting != null && display.isPainting.count() > 0) return;
+	NSString value = ((NSControl) view).stringValue();
+	searchFieldHasText = value != null && (int) value.length() > 0;
+}
+
 @Override
 NSRange textView_willChangeSelectionFromCharacterRange_toCharacterRange (long id, long sel, long aTextView, long oldSelectedCharRange, long newSelectedCharRange) {
 	/*

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java

@@ -13,15 +13,23 @@
  *******************************************************************************/
 package org.eclipse.swt.tests.junit;
 
+import static java.lang.System.currentTimeMillis;
+import static java.lang.System.nanoTime;
 import static org.eclipse.swt.tests.junit.SwtTestUtil.JENKINS_DETECT_ENV_VAR;
 import static org.eclipse.swt.tests.junit.SwtTestUtil.JENKINS_DETECT_REGEX;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+
+import java.lang.management.ManagementFactory;
 
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.events.ModifyListener;
+import org.eclipse.swt.events.PaintListener;
 import org.eclipse.swt.events.SegmentListener;
 import org.eclipse.swt.events.SelectionEvent;
 import org.eclipse.swt.events.SelectionListener;
@@ -30,6 +38,7 @@
 import org.eclipse.swt.graphics.Font;
 import org.eclipse.swt.graphics.FontData;
 import org.eclipse.swt.graphics.Point;
+import org.eclipse.swt.layout.FillLayout;
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Event;
 import org.eclipse.swt.widgets.Group;
@@ -1376,6 +1385,58 @@ public void test_showSelection() {
 	text.showSelection();
 }
 
+// Originally reported as https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+@Test
+public void test_finiteRedrawCancelButtonWithBackground() {
+	if ( text != null ) text.dispose();
+	// Style constants are causing
+	// org.eclipse.swt.widgets.Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long)
+	// to call
+	// org.eclipse.swt.internal.cocoa.NSControl.stringValue()
+	// which schedules redraw
+	text = new Text(shell, SWT.SEARCH | SWT.ICON_CANCEL);
+	// Background prevents early exit from drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long)
+	text.setBackground(shell.getDisplay().getSystemColor(SWT.COLOR_RED));
+	setWidget(text);
+	shell.setLayout(new FillLayout());
+	text.requestLayout();
+	shell.open();
+	text.forceFocus();
+	waitUntilIdle();
+	assertIdle();
+}
+
+@Test
+public void test_finiteRedrawCancelButton() {
+	if ( text != null ) text.dispose();
+	// Style constants are causing
+	// org.eclipse.swt.widgets.Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long)
+	// to call
+	// org.eclipse.swt.internal.cocoa.NSControl.stringValue()
+	// which schedules redraw
+	text = new Text(shell, SWT.SEARCH | SWT.ICON_CANCEL);
+	setWidget(text);
+	shell.setLayout(new FillLayout());
+	text.requestLayout();
+	shell.open();
+	text.forceFocus();
+	waitUntilIdle();
+	assertIdle();
+}
+
+@Test
+public void test_finiteRedraw() {
+	if ( text != null ) text.dispose();
+	text = new Text(shell, SWT.NONE);
+	setWidget(text);
+	shell.setLayout(new FillLayout());
+	text.requestLayout();
+	shell.open();
+	text.forceFocus();
+	waitUntilIdle();
+	assertIdle();
+}
+
 /* custom */
 Text text;
 String delimiterString;
@@ -1639,4 +1700,77 @@ private void pasteFromClipboard(Text text) throws InterruptedException {
 	SwtTestUtil.processEvents(1000, () -> !oldText.equals(text.getText()));
 }
 
+private void waitUntilIdle() {
+	long hangTimeout = currentTimeMillis() + 1000;
+	long lastActive = nanoTime();
+	while (currentTimeMillis() < hangTimeout) {
+		if (shell.getDisplay().readAndDispatch()) {
+			lastActive = nanoTime();
+		} else {
+			// On GTK, blinking caret animation fires with 60 Hz, can't expect long idle times
+			if (lastActive < nanoTime() - 10_000_000) {
+				return;
+			}
+			Thread.yield();
+		}
+	}
+	fail("Unexpected system events keep coming");
+}
+
+private void assertIdle() {
+	assumeFalse(SwtTestUtil.isGTK4(), "GTK4 bug - 10-40% CPU in idle");
+	long[] paintCount = new long[] { 0 };
+	long wallNs, cpuNs;
+	PaintListener paintListener = ignored -> {
+		paintCount[0]++;
+	};
+	text.addPaintListener(paintListener);
+	try {
+		Display display = shell.getDisplay();
+		var tmx = ManagementFactory.getThreadMXBean();
+		assumeTrue(tmx.isThreadCpuTimeSupported() && tmx.isThreadCpuTimeEnabled(),
+				"Thread CPU time measurement is not available on this JVM");
+
+		final int MEASURE_MS = 2000;
+
+		// Schedule a single one-shot timer for the whole measurement window.
+		// When it fires it (a) sets the guard that terminates the loop, and
+		// (b) wakes display.sleep() on platforms that would otherwise block
+		// indefinitely because they generate no background events.
+		final boolean[] done = {false};
+		display.timerExec(MEASURE_MS, () -> done[0] = true);
+
+		long threadId = Thread.currentThread().threadId();
+		long cpuBefore = tmx.getThreadCpuTime(threadId);
+		long wallStart = System.nanoTime();
+		// Additional protection against broken event loop, happens on MacOS SDK 24
+		long stopGuard = currentTimeMillis() + MEASURE_MS * 2;
+		while (!done[0]) {
+			if (stopGuard < currentTimeMillis()) {
+				fail("Timer should fire");
+			}
+			if (!display.readAndDispatch()) {
+				// On GTK, blinking caret animation fires with 60 Hz
+				// We can't just count busy iterations
+				// Hence the actual CPU load measurement below
+				display.sleep();
+			}
+		}
+		wallNs = System.nanoTime() - wallStart;
+
+		cpuNs = tmx.getThreadCpuTime(threadId) - cpuBefore;
+	} finally {
+		text.removePaintListener(paintListener);
+	}
+	double cpuFraction = (double) cpuNs / wallNs;
+	int maxPaint = 15;
+	double maxCPU = 0.05;
+	String message = "CPU usage when idle: %.1f%% < %.1f%%. Paint events: %d < %d"
+			.formatted(cpuFraction * 100, maxCPU * 100, paintCount[0], maxPaint);
+	if (SwtTestUtil.verbose) {
+		System.out.println(message);
+	}
+	assertTrue(cpuFraction < maxCPU && paintCount[0] < maxPaint, message);
+}
+
 }