Bug 568033 - IllegalArgumentException in StyledText.replaceTextRange()

Fixed to no longer throw exception if \r\n were on different lines
already.

Even though the test snippet reproduces exactly the stack as reported by
users, I do feel that users might be having a different problem. This
feeling mostly comes from the fact that we only received reports on
Windows in combination with 'WM_IME_COMPOSITION', while the snippet
reproduces the problem on all 3 platforms.

For this reason, I also changed 'DefaultContent.isValidReplace()' to
throw 3 different exceptions with additional info to help future
debugging if this patch doesn't solve the problem.

I didn't find any outside uses of 'DefaultContent.isValidReplace()', so
I think that it's OK to remove 'protected' and change signature.

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

3 files changed, 164 insertions, 28 deletions

diff --git a/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/DefaultContent.java b/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/DefaultContent.java
index 35450894ba..7c77fc46b2 100644
--- a/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/DefaultContent.java
+++ b/bundles/org.eclipse.swt/Eclipse SWT Custom Widgets/common/org/eclipse/swt/custom/DefaultContent.java
@@ -170,46 +170,55 @@ boolean isDelimiter(char ch) {
 	if (ch == SWT.LF) return true;
 	return false;
 }
+
+private boolean isInsideCRLF(int pos) {
+	if (pos == 0) return false;
+	if (pos == getCharCount()) return false;
+
+	char charBefore = getTextRange(pos - 1, 1).charAt(0);
+	if (charBefore != '\r') return false;
+
+	char charAfter = getTextRange(pos, 1).charAt(0);
+	if (charAfter != '\n') return false;
+
+	/*
+	 * Bug 568033: in case of this.setText("\rxxx\n")
+	 * \r and \n are already parsed as separate line endings, so it
+	 * shouldn't be wrong to delete 'xxx' and type something there.
+	 */
+	if (getLineAtOffset(pos - 1) != getLineAtOffset(pos))
+		return false;
+
+	return true;
+}
+
 /**
- * Determine whether or not the replace operation is valid.  DefaultContent will not allow
- * the /r/n line delimiter to be split or partially deleted.
+ * Validates the replace operation.  DefaultContent will not allow
+ * the \r\n line delimiter to be split or partially deleted.
  * <p>
  *
- * @param start	start offset of text to replace
+ * @param start start offset of text to replace
  * @param replaceLength start offset of text to replace
- * @param newText start offset of text to replace
- * @return a boolean specifying whether or not the replace operation is valid
  */
-protected boolean isValidReplace(int start, int replaceLength, String newText){
+private void validateReplace(int start, int replaceLength) {
 	if (replaceLength == 0) {
 		// inserting text, see if the \r\n line delimiter is being split
-		if (start == 0) return true;
-		if (start == getCharCount()) return true;
-		char before = getTextRange(start - 1, 1).charAt(0);
-		if (before == '\r') {
-			char after = getTextRange(start, 1).charAt(0);
-			if (after == '\n') return false;
+		if (isInsideCRLF(start)) {
+			String message = " [0: start=" + start + " len=" + replaceLength + "]";
+			SWT.error(SWT.ERROR_INVALID_ARGUMENT, null, message);
 		}
 	} else {
 		// deleting text, see if part of a \r\n line delimiter is being deleted
-		char startChar = getTextRange(start, 1).charAt(0);
-		if (startChar == '\n') {
-			// see if char before delete position is \r
-			if (start != 0) {
-				char before = getTextRange(start - 1, 1).charAt(0);
-				if (before == '\r') return false;
-			}
+		if (isInsideCRLF(start)) {
+			String message = " [1: start=" + start + " len=" + replaceLength + "]";
+			SWT.error(SWT.ERROR_INVALID_ARGUMENT, null, message);
 		}
-		char endChar = getTextRange(start + replaceLength - 1, 1).charAt(0);
-		if (endChar == '\r') {
-			// see if char after delete position is \n
-			if (start + replaceLength != getCharCount()) {
-				char after = getTextRange(start + replaceLength, 1).charAt(0);
-				if (after == '\n') return false;
-			}
+
+		if (isInsideCRLF(start + replaceLength)) {
+			String message = " [2: start=" + start + " len=" + replaceLength + "]";
+			SWT.error(SWT.ERROR_INVALID_ARGUMENT, null, message);
 		}
 	}
-	return true;
 }
 /**
  * Calculates the indexes of each line of text in the given range.
@@ -780,7 +789,7 @@ public void removeTextChangeListener(TextChangeListener listener){
 @Override
 public void replaceTextRange(int start, int replaceLength, String newText){
 	// check for invalid replace operations
-	if (!isValidReplace(start, replaceLength, newText)) SWT.error(SWT.ERROR_INVALID_ARGUMENT);
+	validateReplace(start, replaceLength);
 
 	// inform listeners
 	StyledTextEvent event = new StyledTextEvent(this);
diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_custom_StyledText.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_custom_StyledText.java
index 7f7e71377d..f890c6abd6 100644
--- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_custom_StyledText.java
+++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_custom_StyledText.java
@@ -19,6 +19,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeFalse;
@@ -5874,6 +5875,31 @@ public void test_backspaceAndDelete() {
 
 	assertEquals(0, text.getText().length());
 }
+
+/**
+ * Bug 568033 - Splitting CRLF shall not be forbidden if already on different lines
+ */
+@Test
+public void test_replaceTextRange_isInsideCRLF() {
+	text.setText("0123\r\n6789");
+	assertThrows("Exception shall be thrown when splitting CRLF", IllegalArgumentException.class, () -> {
+		text.replaceTextRange(5, 0, "x");
+	});
+
+	assertThrows("Exception shall be thrown when splitting CRLF", IllegalArgumentException.class, () -> {
+		text.replaceTextRange(0, 5, "x");
+	});
+
+	assertThrows("Exception shall be thrown when splitting CRLF", IllegalArgumentException.class, () -> {
+		text.replaceTextRange(5, 5, "x");
+	});
+
+	// Shouldn't throw when CR/LF were already on different lines
+	text.setText("0\r2\n4");
+	text.replaceTextRange(2, 1, "");
+	text.replaceTextRange(2, 0, "2");
+}
+
 private Event keyEvent(int key, int type, Widget w) {
 	Event e = new Event();
 	e.keyCode= key;
diff --git a/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug568033_StyledText_CRLF_IAE.java b/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug568033_StyledText_CRLF_IAE.java
new file mode 100644
index 0000000000..04cb6ae62a
--- /dev/null
+++ b/tests/org.eclipse.swt.tests/ManualTests/org/eclipse/swt/tests/manual/Bug568033_StyledText_CRLF_IAE.java
@@ -0,0 +1,101 @@
+/*******************************************************************************
+ * 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.manual;
+
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.custom.StyledText;
+import org.eclipse.swt.layout.GridData;
+import org.eclipse.swt.layout.GridLayout;
+import org.eclipse.swt.widgets.*;
+
+public class Bug568033_StyledText_CRLF_IAE {
+	public static void main(String[] args) {
+		final Display display = new Display();
+
+		final Shell shell = new Shell(display);
+		shell.setLayout(new GridLayout(1, true));
+
+		final Label hint = new Label(shell, SWT.WRAP);
+		hint.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
+		hint.setText(
+			"Problem: IllegalArgumentException occurs in some scenarios\n" +
+			"\n" +
+			"Reproducing on any platform with keyDown event:\n" +
+			"1) Select 'Selectme' in StyledText below\n" +
+			"2) Hit Backspace to delete it\n" +
+			"3) Press 'A' key\n" +
+			"\n" +
+			"Reproducing with IME on any platform:\n" +
+			"1) Install keyboard layout with IME, such as Chinese Simplified - Pinyin\n" +
+			"2) Select 'Selectme' in StyledText below\n" +
+			"3) Hit Backspace to delete it\n" +
+			"4) Press 'A' key and select any hieroglyph\n" +
+			"\n" +
+			"Reproducing with 'handleCompositionChanged' event with Emoji on Win10:\n" +
+			"1) Select 'Selectme' in StyledText below\n" +
+			"2) Press Win+.\n" +
+			"3) Select any emoji\n"
+		);
+
+		StyledText text = new StyledText(shell, 0);
+		text.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true));
+		// Problem is caused by \r\n separated with some text
+		text.setText("\rSelectme\n");
+
+		final Button button = new Button(shell, 0);
+		button.setText("Test that errors are still caught");
+		button.addListener(SWT.Selection, event -> {
+			text.setText("0123\r\n6789");
+
+			try {
+				text.replaceTextRange(5, 0, "x");
+				System.out.println("Test1: BAD!");
+			} catch (IllegalArgumentException ex) {
+				System.out.println("Test1: ok");
+			}
+
+			try {
+				text.replaceTextRange(0, 5, "x");
+				System.out.println("Test2: BAD!");
+			} catch (IllegalArgumentException ex) {
+				System.out.println("Test2: ok");
+			}
+
+			try {
+				text.replaceTextRange(5, 5, "x");
+				System.out.println("Test3: BAD!");
+			} catch (IllegalArgumentException ex) {
+				System.out.println("Test3: ok");
+			}
+
+			try {
+				text.replaceTextRange(4, 2, "|");
+				System.out.println("Test4: ok");
+			} catch (IllegalArgumentException ex) {
+				System.out.println("Test4: BAD!");
+			}
+		});
+
+		shell.pack();
+		shell.open();
+
+		while (!shell.isDisposed()) {
+			if (!display.readAndDispatch()) {
+				display.sleep();
+			}
+		}
+
+		display.dispose();
+	}
+}