Skip to content

Commit 5839e47

Browse files
Symeon94Symeon94
authored
Resolve #1293 - Partial fix for incorrect caret position after Undo (#1295) (#1295)
1 parent be18bb1 commit 5839e47

4 files changed

Lines changed: 528 additions & 10 deletions

File tree

‎richtextfx/src/integrationTest/java/org/fxmisc/richtext/api/UndoManagerTests.java‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import javafx.beans.property.SimpleIntegerProperty;
55
import javafx.scene.Scene;
66
import javafx.scene.control.Label;
7+
import javafx.scene.input.KeyCode;
78
import javafx.stage.Stage;
89
import org.fxmisc.richtext.InlineCssTextAreaAppTest;
910
import org.fxmisc.richtext.RichTextFXTestBase;
@@ -24,6 +25,37 @@
2425
public class UndoManagerTests {
2526

2627
public class UsingInlineCssTextArea extends InlineCssTextAreaAppTest {
28+
private void checkCaretAtPosition(int position) {
29+
checkSelection(position, position, position);
30+
}
31+
32+
private void checkSelection(int start, int end, int caret) {
33+
assertEquals(start, area.getSelection().getStart());
34+
assertEquals(end, area.getSelection().getEnd());
35+
assertEquals(caret, area.getCaretPosition());
36+
}
37+
38+
private void input(KeyCode... codes) {
39+
press(codes);
40+
release(codes);
41+
}
42+
43+
private void selectNext(int count) {
44+
selectMultiple(count, KeyCode.RIGHT);
45+
}
46+
47+
private void selectPrevious(int count) {
48+
selectMultiple(count, KeyCode.LEFT);
49+
}
50+
51+
private void selectMultiple(int count, KeyCode keyCode) {
52+
press(KeyCode.SHIFT);
53+
for (int i = 0; i < count; i++) {
54+
input(keyCode);
55+
}
56+
release(KeyCode.SHIFT);
57+
58+
}
2759

2860
@Test
2961
public void incoming_change_is_not_merged_after_period_of_user_inactivity() {
@@ -43,11 +75,163 @@ public void incoming_change_is_not_merged_after_period_of_user_inactivity() {
4375
assertEquals("", area.getText());
4476
}
4577

78+
@Test // Perform addition, backspace, delete, replace and check the caret position
79+
public void undo_for_addition() {
80+
// Addition
81+
write("Hat");
82+
assertEquals("Hat", area.getText());
83+
checkCaretAtPosition(3);
84+
85+
// Move caret to 1
86+
area.moveTo(1);
87+
checkCaretAtPosition(1);
88+
89+
// Add a letter
90+
write("e");
91+
assertEquals("Heat", area.getText());
92+
checkCaretAtPosition(2);
93+
94+
// Undo removes " a"
95+
interact(area::undo);
96+
assertEquals("Hat", area.getText());
97+
checkCaretAtPosition(1);
98+
}
99+
100+
@Test // Perform addition, backspace, delete, replace and check the caret position
101+
public void undo_for_delete() {
102+
// Addition
103+
write("Hat");
104+
assertEquals("Hat", area.getText());
105+
checkCaretAtPosition(3);
106+
107+
// Move caret to 1
108+
area.moveTo(1);
109+
checkCaretAtPosition(1);
110+
111+
// Delete
112+
input(KeyCode.DELETE);
113+
assertEquals("Ht",area.getText());
114+
checkCaretAtPosition(1);
115+
116+
// Undo puts back the 'a'
117+
interact(area::undo);
118+
assertEquals("Hat",area.getText());
119+
checkCaretAtPosition(2); // TODO Bug #1293: Correct value is 1 (but creating test to cover existing behaviour)
120+
121+
// Delete with "delete" starting from the start and going to the end
122+
area.moveTo(1);
123+
selectNext(2);
124+
checkSelection(1, 3, 3);
125+
// Backspace deletes
126+
input(KeyCode.DELETE);
127+
assertEquals("H",area.getText());
128+
checkCaretAtPosition(1);
129+
// undo should put back the content and move the caret at the end
130+
interact(area::undo);
131+
assertEquals("Hat",area.getText());
132+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 3);
133+
134+
// Delete with "delete" from the end and going to the start
135+
area.moveTo(3);
136+
selectPrevious(2);
137+
checkSelection(1, 3, 1);
138+
// Backspace deletes
139+
input(KeyCode.DELETE);
140+
assertEquals("H",area.getText());
141+
checkCaretAtPosition(1);
142+
// undo should put back the content and move the caret at the end
143+
interact(area::undo);
144+
assertEquals("Hat",area.getText());
145+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 1);
146+
}
147+
148+
@Test // Perform addition, backspace, delete, replace and check the caret position
149+
public void undo_for_backspace() {
150+
// Addition
151+
write("Hat");
152+
assertEquals("Hat", area.getText());
153+
checkCaretAtPosition(3);
154+
155+
// Move caret to 1
156+
area.moveTo(1);
157+
checkCaretAtPosition(1);
158+
159+
// Backspace
160+
input(KeyCode.BACK_SPACE);
161+
assertEquals("at",area.getText());
162+
checkCaretAtPosition(0);
163+
164+
// Undo puts back the a
165+
interact(area::undo);
166+
assertEquals("Hat",area.getText());
167+
checkCaretAtPosition(1);
168+
169+
// Delete with backspace starting from the start and going to the end
170+
selectNext(2);
171+
checkSelection(1, 3, 3);
172+
// Backspace deletes
173+
input(KeyCode.BACK_SPACE);
174+
assertEquals("H",area.getText());
175+
checkCaretAtPosition(1);
176+
// undo should put back the content and move the caret at the end
177+
interact(area::undo);
178+
assertEquals("Hat",area.getText());
179+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 3)
180+
181+
// Delete with backspace from the end and going to the start
182+
area.moveTo(3);
183+
selectPrevious(2);
184+
checkSelection(1, 3, 1);
185+
// Backspace deletes
186+
input(KeyCode.BACK_SPACE);
187+
assertEquals("H",area.getText());
188+
checkCaretAtPosition(1);
189+
// undo should put back the content and move the caret at the end
190+
interact(area::undo);
191+
assertEquals("Hat",area.getText());
192+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 1)
193+
}
194+
195+
@Test // Perform addition, backspace, delete, replace and check the caret position
196+
public void undo_for_replace() {
197+
// Addition
198+
write("Heat");
199+
assertEquals("Heat", area.getText());
200+
checkCaretAtPosition(4);
201+
202+
// Replace (but first add)
203+
area.selectRange(1, 3);
204+
checkSelection(1, 3, 3);
205+
// press a key to replace the text
206+
input(KeyCode.I);
207+
assertEquals("Hit",area.getText());
208+
checkCaretAtPosition(2);
209+
// undo should put back the content and move the caret at the end
210+
interact(area::undo);
211+
assertEquals("Heat",area.getText());
212+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 3);
213+
214+
// Now perform the same operation but the caret of the selection is at the start
215+
area.moveTo(3);
216+
selectPrevious(2);
217+
checkSelection(1, 3, 1);
218+
// press a key to replace the text
219+
input(KeyCode.I);
220+
assertEquals("Hit",area.getText());
221+
checkCaretAtPosition(2);
222+
// undo should put back the content and move the caret at the end
223+
interact(area::undo);
224+
assertEquals("Heat",area.getText());
225+
checkCaretAtPosition(3); // TODO Bug #1293: Correct value is: checkSelection(1, 3, 1);
226+
}
227+
46228
@Test // After undo, text insertion point jumps to the start of the text area #780
47229
// After undo, text insertion point jumps to the end of the text area #912
48230
public void undo_leaves_correct_insertion_point() {
49231

50232
write("abc mno");
233+
// ^
234+
assertEquals(7, area.getCaretPosition());
51235
interact(() -> {
52236
area.insertText(3," def");
53237
area.appendText(" xyz");

‎richtextfx/src/main/java/org/fxmisc/richtext/util/UndoUtils.java‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTex
9797
public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTextUndoManager(
9898
GenericStyledArea<PS, SEG, S> area, Duration preventMergeDelay) {
9999
return richTextUndoManager(area, UndoManagerFactory.unlimitedHistoryFactory(), preventMergeDelay);
100-
};
100+
}
101101

102102
/**
103103
* Returns an UndoManager that can undo/redo {@link RichTextChange}s. New changes
@@ -107,7 +107,7 @@ public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTex
107107
public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTextUndoManager(
108108
GenericStyledArea<PS, SEG, S> area, UndoManagerFactory factory) {
109109
return richTextUndoManager(area, factory, DEFAULT_PREVENT_MERGE_DELAY);
110-
};
110+
}
111111

112112
/**
113113
* Returns an UndoManager that can undo/redo {@link RichTextChange}s. New changes
@@ -122,7 +122,7 @@ public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTex
122122
TextChange::mergeWith,
123123
TextChange::isIdentity,
124124
preventMergeDelay);
125-
};
125+
}
126126

127127
/**
128128
* Returns an UndoManager with an unlimited history that can undo/redo {@link RichTextChange}s. New changes
@@ -154,7 +154,7 @@ public static <PS, SEG, S> UndoManager<List<RichTextChange<PS, SEG, S>>> richTex
154154
area.multiRichChanges().conditionOn(suspendUndo),
155155
preventMergeDelay
156156
);
157-
};
157+
}
158158

159159
/**
160160
* Returns an UndoManager with an unlimited history that can undo/redo {@link PlainTextChange}s. New changes
@@ -270,10 +270,18 @@ public static <PS, SEG, S> Consumer<List<RichTextChange<PS, SEG, S>>> applyMulti
270270
*/
271271
private static void moveToChange( GenericStyledArea area, TextChange chg )
272272
{
273-
int pos = chg.getPosition();
274-
int len = chg.getNetLength();
275-
if ( len > 0 ) pos += len;
276-
273+
// Issue #1293: This is still not sufficient for undo because:
274+
// - You want to restore the selection after undo, information is missing.
275+
// - Removing a character with "DEL" or "BACKSPACE" result in the same change but the caret should not be at the
276+
// same position when undoing one compared to the other.
277+
// - You can select left-to-right or right-to-left, but without the position of the caret before
278+
// the action we cannot put back the caret where it used to be.
279+
int pos = chg.getInsertionEnd();
280+
/* Before fixing #1293 (remove this comment when the issue is fully fixed):
281+
* int pos = chg.getPosition();
282+
* int len = chg.getNetLength();
283+
* if ( len > 0 ) pos += len;
284+
*/
277285
area.moveTo( Math.min( pos, area.getLength() ) );
278286
}
279287
}

0 commit comments

Comments
 (0)
⚡