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>
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 3545089..7c77fc4 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 @@
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 @@
@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 7f7e713..f890c6a 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.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 @@
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 0000000..04cb6ae
--- /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();
+ }
+}
