Conversation
…VirtualFlow. Only the fields from the ViewModel that have been changed and that are relevant for the UI need to be synced. Fix gluonhq#429
There was a problem hiding this comment.
Pull request overview
This PR optimizes the text rendering performance by replacing the refreshTextFlow() call with a new, more lightweight textBufferChanged() method in response to TextBuffer events. The goal is to avoid rebuilding the entire text flow (VF) on every change in the backing TextBuffer.
Key changes:
- Modified the textChangeListener to call
textBufferChanged(e)instead ofrefreshTextFlow() - Added a new
textBufferChanged()method that performs minimal operations: resetting nonTextNodes counter, resetting the character iterator, and conditionally requesting layout only when the number of non-text nodes changes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nonTextNodes.set(0); | ||
| viewModel.resetCharacterIterator(); | ||
| if (nonTextNodesCount != nonTextNodes.get()) { | ||
| // when number of images changes, caret |
There was a problem hiding this comment.
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.
| nonTextNodes.set(0); | ||
| viewModel.resetCharacterIterator(); | ||
| if (nonTextNodesCount != nonTextNodes.get()) { | ||
| // when number of images changes, caret | ||
| requestLayout(); | ||
| nonTextNodesCount = nonTextNodes.get(); |
There was a problem hiding this comment.
The new textBufferChanged method is missing several important operations that were present in refreshTextFlow:
- paragraphListView.updateLayout() - which ensures decoration changes are applied
- getSkinnable().requestFocus() - which requests focus on the component
- 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.
| 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(); |
| private void textBufferChanged(TextBuffer.Event e) { | ||
| nonTextNodes.set(0); | ||
| viewModel.resetCharacterIterator(); | ||
| if (nonTextNodesCount != nonTextNodes.get()) { | ||
| // when number of images changes, caret | ||
| requestLayout(); | ||
| nonTextNodesCount = nonTextNodes.get(); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| private final SmartTimer objectsCacheEvictionTimer; | ||
|
|
||
| private final Consumer<TextBuffer.Event> textChangeListener = e -> refreshTextFlow(); | ||
| private final Consumer<TextBuffer.Event> textChangeListener = e -> textBufferChanged(e); |
There was a problem hiding this comment.
The lambda expression can be simplified to a method reference for better readability.
| private final Consumer<TextBuffer.Event> textChangeListener = e -> textBufferChanged(e); | |
| private final Consumer<TextBuffer.Event> textChangeListener = this::textBufferChanged; |
Do not rebuild the VF on every change in the backing TextBuffer.