Bug 569529 - [Cocoa] Link and multi-line Text hog CPU in dark theme

The key change is to use 'lastAppAppearance' to avoid endless redraws.

Tests for 'APPEARANCE.Dark' were removed in anticipation of fixing
'OS.setTheme()' to be able to switch theme dynamically. I understand
that colors need to be updated again when switching fro mDark to Light.

Colors are updated *before* calling 'super', because it's only
reasonable to do the drawing with new colors already.

'Text.drawBackground()' is called from 'Text.drawRect()', so it doesn't
need a color adjustment of its own.

Change-Id: If504c4dd5da1a0984f75829e81efa507d41763d4
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>

3 files changed, 249 insertions, 42 deletions

diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Link.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Link.java
index 2cd4e71842..ae364618b7 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Link.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Link.java
@@ -52,6 +52,7 @@ public class Link extends Control {
 	NSColor defaultLinkColor;
 	int focusIndex;
 	boolean ignoreNextMouseUp;
+	APPEARANCE lastAppAppearance;
 
 /**
  * Constructs a new instance of this class given its parent
@@ -247,10 +248,8 @@ void drawBackground (long id, NSGraphicsContext context, NSRect rectangle) {
 
 @Override
 void drawRect(long id, long sel, NSRect rect) {
+	updateThemeColors();
 	super.drawRect(id, sel, rect);
-	if (display.appAppearance == APPEARANCE.Dark) {
-		setDefaultForeground();
-	}
 }
 
 @Override
@@ -670,15 +669,6 @@ void setBackgroundImage(NSImage image) {
 	((NSTextView) view).setDrawsBackground(image == null);
 }
 
-void setDefaultForeground() {
-	if (foreground != null) return;
-	if (getEnabled ()) {
-		((NSTextView) view).setTextColor (NSColor.textColor ());
-	} else {
-		((NSTextView) view).setTextColor (NSColor.disabledControlTextColor ());
-	}
-}
-
 @Override
 void setFont(NSFont font) {
 	((NSTextView) view).setFont(font);
@@ -859,5 +849,26 @@ void updateCursorRects (boolean enabled) {
 	contentView.setDocumentCursor (enabled ? NSCursor.arrowCursor () : null);
 }
 
+void updateThemeColors() {
+	/*
+	 * On macOS 10.14 and 10.15, when application sets Dark appearance, NSTextView
+	 * does not change the text color. In case of the link, this means that text
+	 * outside <a></a> will be black-on-dark. Fix this by setting the text color
+	 * explicitly. It seems that this is no longer needed on macOS 11.0. Note that
+	 * there is 'setUsesAdaptiveColorMappingForDarkAppearance:' which causes
+	 * NSTextView to adapt its colors, but it will also remap any colors used in
+	 * .setBackground(), which makes it difficult to use. I wasn't able to find an
+	 * event that colors changed, 'drawRect' seems to be the best option.
+	 */
+
+	// Avoid infinite loop of redraws
+	if (lastAppAppearance == display.appAppearance) return;
+	lastAppAppearance = display.appAppearance;
+	// Only default colors are affected
+	if (foreground != null) return;
+
+	((NSTextView) view).setTextColor (getTextColor (getEnabled ()));
+}
+
 }
 
diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java
index 0629f85850..9090131cc2 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java
@@ -68,6 +68,7 @@ public class Text extends Scrollable {
 	NSRange selectionRange;
 	id targetSearch, targetCancel;
 	long actionSearch, actionCancel;
+	APPEARANCE lastAppAppearance;
 
 	/**
 	* The maximum number of characters that can be entered
@@ -691,24 +692,6 @@ void drawBackground (long id, NSGraphicsContext context, NSRect rect) {
 		}
 	} else if ((style & SWT.MULTI) != 0) {
 		if (id != scrollView.id) return;
-
-		/*
-		 *  If background image has to be set, call fillBackground(). Else, set background color
-		 *  here directly on the NSTextView and return.
-		 */
-		if (backgroundImage == null) {
-			double [] background = this.background;
-			double alpha;
-			if (background == null) {
-				background = defaultBackground ().handle;
-				alpha = getThemeAlpha ();
-			} else {
-				alpha = background[3];
-			}
-			NSColor nsColor = NSColor.colorWithDeviceRed(background[0], background[1], background[2], alpha);
-			((NSTextView) view).setBackgroundColor(nsColor);
-			return;
-		}
 	}
 	fillBackground (view, context, rect, -1);
 }
@@ -785,10 +768,8 @@ void drawInteriorWithFrame_inView_searchfield (long id, long sel, NSRect cellFra
 
 @Override
 void drawRect(long id, long sel, NSRect rect) {
+	updateThemeColors();
 	super.drawRect(id, sel, rect);
-	if (display.appAppearance == APPEARANCE.Dark) {
-		setDefaultForeground();
-	}
 }
 
 @Override
@@ -1861,16 +1842,10 @@ void setBackgroundImage(NSImage image) {
 	} else {
 		((NSTextView) view).setDrawsBackground(image == null);
 		scrollView.setDrawsBackground(image == null);
-	}
-}
 
-void setDefaultForeground() {
-	if ((style & SWT.MULTI) != 0) {
-		if (foreground != null) return;
-		if (getEnabled ()) {
-			((NSTextView) view).setTextColor (NSColor.textColor ());
-		} else {
-			((NSTextView) view).setTextColor (NSColor.disabledControlTextColor ());
+		if (image == null) {
+			// Recalculate theme colors lazily
+			lastAppAppearance = null;
 		}
 	}
 }
@@ -2519,6 +2494,31 @@ void updateCursorRects (boolean enabled) {
 	contentView.setDocumentCursor (enabled ? NSCursor.IBeamCursor () : null);
 }
 
+void updateThemeColors() {
+	// See code comment in Link.updateThemeColors() for explanation
+
+	// Avoid infinite loop of redraws
+	if (lastAppAppearance == display.appAppearance) return;
+	lastAppAppearance = display.appAppearance;
+	// Only multi-line controls are affected
+	if ((style & SWT.MULTI) == 0) return;
+
+	if (foreground == null) {
+		if (getEnabled ()) {
+			((NSTextView) view).setTextColor (NSColor.textColor ());
+		} else {
+			((NSTextView) view).setTextColor (NSColor.disabledControlTextColor ());
+		}
+	}
+
+	if ((backgroundImage == null) && (background == null)) {
+		double [] background = defaultBackground ().handle;
+		double alpha = getThemeAlpha ();
+		NSColor nsColor = NSColor.colorWithDeviceRed(background[0], background[1], background[2], alpha);
+		((NSTextView) view).setBackgroundColor(nsColor);
+	}
+}
+
 String verifyText (String string, int start, int end, NSEvent keyEvent) {
 	Event event = new Event ();
 	if (keyEvent != null) setKeyState(event, SWT.MouseDown, keyEvent);
diff --git a/tests/org.eclipse.swt.tests.cocoa/ManualTests/org/eclipse/swt/tests/cocoa/snippets/Bug569529_macOS_LinkAndTextHogCPU.java b/tests/org.eclipse.swt.tests.cocoa/ManualTests/org/eclipse/swt/tests/cocoa/snippets/Bug569529_macOS_LinkAndTextHogCPU.java
new file mode 100644
index 0000000000..056b47c6c5
--- /dev/null
+++ b/tests/org.eclipse.swt.tests.cocoa/ManualTests/org/eclipse/swt/tests/cocoa/snippets/Bug569529_macOS_LinkAndTextHogCPU.java
@@ -0,0 +1,196 @@
+/*******************************************************************************
+ * Copyright (c) 2020 Syntevo and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Syntevo - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.swt.tests.cocoa.snippets;
+
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.graphics.Color;
+import org.eclipse.swt.internal.cocoa.OS;
+import org.eclipse.swt.layout.GridLayout;
+import org.eclipse.swt.widgets.*;
+
+import java.util.ArrayList;
+
+public class Bug569529_macOS_LinkAndTextHogCPU {
+	public static void main(String[] args) {
+		System.setProperty("org.eclipse.swt.display.useSystemTheme", "true");
+
+		final Display display = new Display();
+		final Shell shell = new Shell(display);
+		shell.setLayout(new GridLayout(1, true));
+
+		final Label hint = new Label(shell, 0);
+		hint.setText(
+			"1) Use macOS with Dark Theme\n" +
+			"2) Notice that snippet constantly hogs CPU\n" +
+			"3) On macOS 10.15, hiding controls helps but not on BigSur\n" +
+			"4) Destroying controls helps"
+		);
+
+		final Composite compColumns = new Composite(shell, 0);
+		compColumns.setLayout(new GridLayout(2, true));
+
+		{
+			final Composite parent = new Composite(compColumns, 0);
+			parent.setLayout(new GridLayout(1, true));
+
+			final Composite itemsParent = new Composite(parent, 0);
+			itemsParent.setLayout(new GridLayout(1, true));
+
+			final ArrayList<Link> items = new ArrayList<>();
+			final Runnable createDestroy = () -> {
+				if (items.size() != 0) {
+					for (Link item : items)
+					{
+						item.dispose();
+					}
+					items.clear();
+				} else {
+					for (int i = 0; i < 10; i++)
+					{
+						Link item = new Link(itemsParent, 0);
+						items.add(item);
+						item.setText("This is <a>Link#" + i + "</a>");
+					}
+
+					itemsParent.layout(true, true);
+				}
+			};
+
+			Button btnCreateDestroy = new Button(parent, SWT.PUSH);
+			btnCreateDestroy.setText("Create/Destroy");
+			btnCreateDestroy.addListener(SWT.Selection, event -> {
+				createDestroy.run();
+			});
+
+			Button btnShowHide = new Button(parent, SWT.PUSH);
+			btnShowHide.setText("Show/hide");
+			btnShowHide.addListener(SWT.Selection, event -> {
+				for (Link item : items)
+				{
+					item.setVisible(!item.getVisible());
+				}
+			});
+
+			Button btnEnableDisable = new Button(parent, SWT.PUSH);
+			btnEnableDisable.setText("Enable/disable");
+			btnEnableDisable.addListener(SWT.Selection, event -> {
+				for (Link item : items)
+				{
+					item.setEnabled(!item.getEnabled());
+				}
+			});
+
+			Button btnSetRemoveColors = new Button(parent, SWT.PUSH);
+			btnSetRemoveColors.setText("Set/remove colors");
+			final boolean[] hasColors = new boolean[]{false};
+			btnSetRemoveColors.addListener(SWT.Selection, event -> {
+				hasColors[0] = !hasColors[0];
+				for (Link item : items)
+				{
+					if (!hasColors[0]) {
+						item.setForeground(null);
+						item.setBackground(null);
+					} else {
+						item.setForeground(display.getSystemColor(SWT.COLOR_RED));
+						item.setBackground(display.getSystemColor(SWT.COLOR_GREEN));
+					}
+				}
+			});
+
+			createDestroy.run();
+		}
+
+		{
+			final Composite parent = new Composite(compColumns, 0);
+			parent.setLayout(new GridLayout(1, true));
+
+			final Composite itemsParent = new Composite(parent, 0);
+			itemsParent.setLayout(new GridLayout(1, true));
+
+			final ArrayList<Text> items = new ArrayList<>();
+			final Runnable createDestroy = () -> {
+				if (items.size() != 0) {
+					for (Text item : items)
+					{
+						item.dispose();
+					}
+					items.clear();
+				} else {
+					for (int i = 0; i < 10; i++)
+					{
+						Text item = new Text(itemsParent, SWT.MULTI);
+						items.add(item);
+						item.setText("This is Text#" + i);
+					}
+
+					itemsParent.layout(true, true);
+				}
+			};
+
+			Button btnCreateDestroy = new Button(parent, SWT.PUSH);
+			btnCreateDestroy.setText("Create/Destroy");
+			btnCreateDestroy.addListener(SWT.Selection, event -> {
+				createDestroy.run();
+			});
+
+			Button btnShowHide = new Button(parent, SWT.PUSH);
+			btnShowHide.setText("Show/hide");
+			btnShowHide.addListener(SWT.Selection, event -> {
+				for (Text item : items)
+				{
+					item.setVisible(!item.getVisible());
+				}
+			});
+
+			Button btnEnableDisable = new Button(parent, SWT.PUSH);
+			btnEnableDisable.setText("Enable/disable");
+			btnEnableDisable.addListener(SWT.Selection, event -> {
+				for (Text item : items)
+				{
+					item.setEnabled(!item.getEnabled());
+				}
+			});
+
+			Button btnSetRemoveColors = new Button(parent, SWT.PUSH);
+			btnSetRemoveColors.setText("Set/remove colors");
+			final boolean[] hasColors = new boolean[]{false};
+			btnSetRemoveColors.addListener(SWT.Selection, event -> {
+				hasColors[0] = !hasColors[0];
+				for (Text item : items)
+				{
+					if (!hasColors[0]) {
+						item.setForeground(null);
+						item.setBackground(null);
+					} else {
+						item.setForeground(display.getSystemColor(SWT.COLOR_RED));
+						item.setBackground(display.getSystemColor(SWT.COLOR_GREEN));
+					}
+				}
+			});
+
+			createDestroy.run();
+		}
+
+		shell.pack();
+		shell.open();
+
+		while (!shell.isDisposed()) {
+			if (!display.readAndDispatch()) {
+				display.sleep();
+			}
+		}
+
+		display.dispose();
+	}
+}
\ No newline at end of file