Skip to content

Refactor caret and selection code #1309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Symeon94 wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Refactor caret and selection code #1309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a95778
Refactoring
Dec 23, 2025
6d7fa0a
Refactoring
Dec 23, 2025
6c1f288
Cleanup
Dec 23, 2025
7c3d035
Refactoring
Dec 23, 2025
258f005
Refactor
Dec 23, 2025
6c28a07
Improving clarity
Dec 23, 2025
122e7a4
Refactor
Dec 23, 2025
c40cca4
Merge
Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion richtextfx/src/main/java/org/fxmisc/richtext/CaretNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public CaretNode(String name, GenericStyledArea<?, ?, ?> area, SuspendableNo dep
}

private void handleContentChange(List<PlainTextChange> list) {
int newPosition = new CaretPositionChange().apply(getPosition(), list);
int newPosition = new CaretPositionChange(getPosition()).apply(list);
if (newPosition != getPosition()) {
moveTo(newPosition);
}
Expand Down
64 changes: 36 additions & 28 deletions richtextfx/src/main/java/org/fxmisc/richtext/CaretPositionChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,40 +5,48 @@
import java.util.List;

class CaretPositionChange {
private int position;

/** @param position the current caret position */
public CaretPositionChange(int position) {
this.position = position;
}

/**
* Goes through the list of change and update the caret position accordingly
* @param position the current caret position
* @param list the list of changes
* @return the new caret position
*/
public int apply(int position, List<PlainTextChange> list) {
for (PlainTextChange plainTextChange : list) {
int netLength = plainTextChange.getNetLength();
if (netLength != 0) {
int indexOfChange = plainTextChange.getPosition();
// in case of a replacement: "hello there" -> "hi."
int endOfChange = indexOfChange + Math.abs(netLength);

/*
"->" means add (positive) netLength to position
"<-" means add (negative) netLength to position
"x" means don't update position
public int apply(List<PlainTextChange> changes) {
changes.forEach(this::applyFor);
return position;
}

"+c" means caret was included in the deleted portion of content
"-c" means caret was not included in the deleted portion of content
Before/At/After means indexOfChange "<" / "==" / ">" position
private void applyFor(PlainTextChange plainTextChange) {
int netLength = plainTextChange.getNetLength();
if (netLength != 0) {
int changeStart = plainTextChange.getPosition();
int changeEnd = changeStart + Math.abs(netLength);
position = applyChange(position, changeStart, changeEnd, netLength);
}
}

| Before +c | Before -c | At | After
-------+---------------+-----------+----+------
Add | N/A | -> | -> | x
Delete | indexOfChange | <- | x | x
*/
if (indexOfChange == position && netLength > 0) {
position = position + netLength;
} else if (indexOfChange < position) {
position = position < endOfChange ? indexOfChange : position + netLength;
}
}
/**
* <ul>
* <li>If the position is after the interval of the change, the caret moves left/right if it's a net removal/addition.</li>
* <li>If it's within the change interval, the caret move back at the start of the change.</li>
* <li>If it's equal to the start position, the caret is moved at the end of the changed (it cannot be moved backwards
* if it's a removal of text, hence capping to 0).</li>
* </ul>
*/
private static int applyChange(int position, int changeStart, int changeEnd, int netLength) {
Copy link
Copy Markdown
Collaborator

@Jugen Jugen Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following are just my preferences to making the code easier to understand. If you have strong reasons for not using them, that's fine:

  1. remove static: it seems unnecessary, as it's not for instance being used by a test. Also see next ....
  2. change return to void: position is a field so update it directly. It's confusing, for me a least, when the value is being changed here, then returned, only to update the field by the caller. The only reason it's like this at the moment is because the method is static.
  3. remove the position parameter: it's a field variable so doesn't need to be passed all the time. Again, the only reason it's like this is because the method is static.
  4. if you decide to keep the position parameter then please give it a different name to distinguish it from the field variable.

Copy link
Copy Markdown
Collaborator Author

@Symeon94 Symeon94 Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both a method or a static function are good solutions here, I don't have a strong preference. I'm totally ok changing to a method (and your comments 2/3/4 makes sense with that change).

I'll do ti.

To add some context, I initially extracted this bit of code in CaretPositionChange and SelectionChange to show that it was duplicated code. Then I adapted with the last bit which cannot be done in SelectionChange:

// ...
else if(position == changeStart) {
    position += Math.max(0, netLength);
}

Copy link
Copy Markdown
Collaborator Author

@Symeon94 Symeon94 Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, sorry to contradict myself, I just made the change and then looked at SelectionChange where I cannot make it a method (because it applies to two different values). So I propose that we keep the static to keep evident the close correspondance between the code in these two classes, if that's ok for you.

if(position >= changeEnd) {
position += netLength;
}
else if(position > changeStart) {
position = changeStart;
}
else if(position == changeStart) {
position += Math.max(0, netLength);
}
return position;
}
Expand Down
67 changes: 31 additions & 36 deletions richtextfx/src/main/java/org/fxmisc/richtext/SelectionChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@

import java.util.List;

// TODO SMA can it be that CaretPositionChange is the same as this one but for start == end ?
class SelectionChange {
private int start, end;

// TODO -> to be replaced by state of this class start(), end()
public static class Range {
private final int start, end;
Expand All @@ -23,45 +26,37 @@ public int end() {
}
}

public Range apply(List<PlainTextChange> list, int selectStart, int selectEnd) {
for (PlainTextChange plainTextChange : list) {
int changeLength = plainTextChange.getNetLength();
int indexOfChange = plainTextChange.getPosition();
// in case of a replacement: "hello there" -> "hi."
int endOfChange = indexOfChange + Math.abs(changeLength);

/*
"->" means add (positive) netLength to position
"<-" means add (negative) netLength to position
"x" means don't update position
public SelectionChange(int start, int end) {
this.start = start;
this.end = end;
}

"start / end" means what should be done in each case for each anchor if they differ
public Range applyFor(List<PlainTextChange> changes) {
changes.forEach(this::applyFor);
return new Range(start, end);
}

"+a" means one of the anchors was included in the deleted portion of content
"-a" means one of the anchors was not included in the deleted portion of content
Before/At/After means indexOfChange "<" / "==" / ">" position
private void applyFor(PlainTextChange plainTextChange) {
int netLength = plainTextChange.getNetLength();
int changeStart = plainTextChange.getPosition();
int changeEnd = changeStart + Math.abs(netLength);
if (start == changeStart) {
start += Math.max(netLength, 0);
}
else {
start = applyChange(start, changeStart, changeEnd, netLength);
}
end = applyChange(end, changeStart, changeEnd, netLength);
start = Math.min(start, end);
}

| Before +a | Before -a | At | After
-------+---------------+-----------+--------+------
Add | N/A | -> | -> / x | x
Delete | indexOfChange | <- | x | x
*/
if (indexOfChange == selectStart && changeLength > 0) {
selectStart = selectStart + changeLength;
} else if (indexOfChange < selectStart) {
selectStart = selectStart < endOfChange
? indexOfChange
: selectStart + changeLength;
}
if (indexOfChange < selectEnd) {
selectEnd = selectEnd < endOfChange
? indexOfChange
: selectEnd + changeLength;
}
if (selectStart > selectEnd) {
selectStart = selectEnd;
}
private static int applyChange(int position, int changeStart, int changeEnd, int netLength) {
if(position >= changeEnd) {
position += netLength;
}
else if(position > changeStart) {
position = changeStart;
}
return new Range(selectStart, selectEnd);
return position;
}
}
2 changes: 1 addition & 1 deletion richtextfx/src/main/java/org/fxmisc/richtext/SelectionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ public SelectionImpl(String name, GenericStyledArea<PS, SEG, S> area, int startP
}

private void handleChange(List<PlainTextChange> list) {
SelectionChange.Range newSelection = new SelectionChange().apply(list, getStartPosition(), getEndPosition());
SelectionChange.Range newSelection = new SelectionChange(getStartPosition(), getEndPosition()).applyFor(list);
selectRange(newSelection.start(), newSelection.end());
}

Expand Down
2 changes: 1 addition & 1 deletion richtextfx/src/test/java/org/fxmisc/richtext/CaretPositionChangeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
public class CaretPositionChangeTest {
private void checkChange(int before, int after, PlainTextChange... changes) {
List<PlainTextChange> changeList = Arrays.stream(changes).collect(Collectors.toList());
assertEquals(after, new CaretPositionChange().apply(before, changeList));
assertEquals(after, new CaretPositionChange(before).apply(changeList));
}

@ParameterizedTest
Expand Down
2 changes: 1 addition & 1 deletion richtextfx/src/test/java/org/fxmisc/richtext/SelectionChangeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
public class SelectionChangeTest {
private void checkChange(int startBefore, int endBefore, int startAfter, int endAfter, PlainTextChange... changes) {
List<PlainTextChange> changeList = Arrays.stream(changes).collect(Collectors.toList());
SelectionChange.Range range = new SelectionChange().apply(changeList, startBefore, endBefore);
SelectionChange.Range range = new SelectionChange(startBefore, endBefore).applyFor(changeList);
assertEquals(startAfter, range.start(), "Start does not match");
assertEquals(endAfter, range.end(), "End does not match");
}
Expand Down