Skip to content

don't rebuild VF #430

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
johanvos wants to merge 10 commits into
base: main
Choose a base branch
from
Open

don't rebuild VF #430

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9a7f789
Use an append-only buffer for the additions in the piecetable.
johanvos Dec 10, 2025
6a92e49
Merge branch 'main' into 421-appendonly
johanvos Dec 12, 2025
bb157cb
Resolve conflict
johanvos Dec 12, 2025
145f766
Leverage increasing character of array to use binarysearch
johanvos Dec 12, 2025
099eb6e
use size for unitlist instead of []
johanvos Dec 13, 2025
f657014
Do not layout ParagraphTile outside a pulse.
johanvos Dec 13, 2025
9761719
Don't unconditionally compute parameters of LOG.info();
johanvos Dec 14, 2025
912234c
When the backing TextBuffer changes, we should not rebuild the whole …
johanvos Dec 14, 2025
fcd1f47
Merge branch 'main' into 429-novfrebuild
johanvos Dec 15, 2025
8c43cd7
cleanup
johanvos Dec 15, 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
11 changes: 10 additions & 1 deletion rta/src/main/java/com/gluonhq/richtextarea/RichTextAreaSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ interface ActionBuilder extends Function<KeyEvent, ActionCmd>{}
private final Map<String, Image> imageCache = new ConcurrentHashMap<>();
private final SmartTimer objectsCacheEvictionTimer;

private final Consumer<TextBuffer.Event> textChangeListener = e -> refreshTextFlow();
private final Consumer<TextBuffer.Event> textChangeListener = e -> textBufferChanged(e);
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The lambda expression can be simplified to a method reference for better readability.

Suggested change
private final Consumer<TextBuffer.Event> textChangeListener = e -> textBufferChanged(e);
private final Consumer<TextBuffer.Event> textChangeListener = this::textBufferChanged;

Copilot uses AI. Check for mistakes.
int lastValidCaretPosition = -1;
int mouseDragStart = -1;
int dragAndDropStart = -1;
Expand Down Expand Up @@ -834,6 +834,15 @@ private void updatePromptNodeLocation() {
promptNode.relocate(x, origin.getY());
}

private void textBufferChanged(TextBuffer.Event e) {
nonTextNodes.set(0);
viewModel.resetCharacterIterator();
if (nonTextNodesCount != nonTextNodes.get()) {
// when number of images changes, caret
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The comment is incomplete and unclear. It starts with "when number of images changes, caret" but doesn't finish the thought. Consider completing this comment to explain what happens to the caret when the number of images changes.

Copilot uses AI. Check for mistakes.
requestLayout();
nonTextNodesCount = nonTextNodes.get();
Comment on lines +838 to +843
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The new textBufferChanged method is missing several important operations that were present in refreshTextFlow:

  1. paragraphListView.updateLayout() - which ensures decoration changes are applied
  2. getSkinnable().requestFocus() - which requests focus on the component
  3. The objectsCacheEvictionTimer pause/start logic for managing cache eviction

Without these, text buffer changes may not properly update the UI (decoration changes won't be applied), focus behavior may be incorrect, and the cache eviction timer won't be properly managed. Consider whether these operations are intentionally omitted or if they should be included in this method.

Suggested change
nonTextNodes.set(0);
viewModel.resetCharacterIterator();
if (nonTextNodesCount != nonTextNodes.get()) {
// when number of images changes, caret
requestLayout();
nonTextNodesCount = nonTextNodes.get();
objectsCacheEvictionTimer.pause();
try {
nonTextNodes.set(0);
viewModel.resetCharacterIterator();
// this ensures changes in decoration are applied:
paragraphListView.updateLayout();
computeFullHeight();
if (nonTextNodesCount != nonTextNodes.get()) {
// when number of images changes, caret
requestLayout();
nonTextNodesCount = nonTextNodes.get();
}
getSkinnable().requestFocus();
} finally {
objectsCacheEvictionTimer.start();

Copilot uses AI. Check for mistakes.
}
}
Comment on lines +837 to +845
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

The Event parameter 'e' is not used in this method, but there are different event types (InsertEvent, DeleteEvent, DecorateEvent) available in TextBuffer.Event. Consider using the event type to determine whether certain operations are needed. For example, DecorateEvent might specifically require paragraphListView.updateLayout() to ensure decoration changes are applied, while InsertEvent and DeleteEvent might not.

Copilot uses AI. Check for mistakes.
// TODO Need more optimal way of rendering text fragments.
// For now rebuilding the whole text flow
private void refreshTextFlow() {
Expand Down