Throttle TextChanged to drastically decrease delay for fast typers
#5
Conversation
…nt to look into options that allow for larger throttles)
WalkthroughAdds a debounced two-step text-change flow to GuiElementTextArea: TextChanged now enqueues a main-thread task TextChangedBatched and sets a private debounce flag; TextChangedBatched recalculates auto-height/world bounds, calls base.TextChanged, and clears the flag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TA as GuiElementTextArea
participant UI as MainThread
participant Base as BaseGuiElement
User->>TA: Text input
TA->>TA: TextChanged()\n(check textChangedBatchedQueued)
alt not queued
TA->>TA: set textChangedBatchedQueued = true
TA->>UI: enqueue TextChangedBatched()
else queued
TA-->>User: return (debounced)
end
UI->>TA: TextChangedBatched()
rect rgba(230,245,255,0.5)
note over TA: If Autoheight enabled\nupdate auto-height bounds
TA->>TA: Recalculate world bounds
TA->>Base: base.TextChanged()
end
TA->>TA: textChangedBatchedQueued = false (finally)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
VTMLEditor/GuiElements/Vanilla/GuiElementTextArea.cs (1)
38-38: Consider adding a comment to explain the debounce mechanism.The field name
textChangeDelayeddoesn't fully convey the coalescing behavior where rapid text changes are batched together. A brief comment would help future maintainers understand the intent.Example:
+ // Prevents redundant text processing by batching rapid updates private bool textChangeDelayed;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
VTMLEditor/GuiElements/Vanilla/GuiElementTextArea.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
VTMLEditor/GuiElements/Vanilla/GuiElementTextArea.cs (2)
VTMLEditor/GuiElements/Vanilla/GuiElementEditableTextBase.cs (1)
TextChanged(552-557)VTMLEditor/GuiElements/Vanilla/GuiElementTextBase.cs (1)
GetMultilineTextHeight(49-52)
| internal override void TextChanged() | ||
| { | ||
| if(textChangeDelayed) return; | ||
| textChangeDelayed = true; | ||
| api.Event.EnqueueMainThreadTask(TextChangedDelayed, "TextChangedDelayed"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Missing exception handling could permanently disable text change processing.
If TextChangedDelayed() throws an exception before line 55 executes, the textChangeDelayed flag will remain true indefinitely, causing all subsequent text changes to be silently ignored. This would completely break the text area functionality.
Additionally, verify that TextChanged() is only called from the main thread. If multiple threads can invoke this method concurrently, the check-and-set pattern at lines 42-43 creates a race condition.
Apply this diff to add exception safety:
internal override void TextChanged()
{
if(textChangeDelayed) return;
textChangeDelayed = true;
- api.Event.EnqueueMainThreadTask(TextChangedDelayed, "TextChangedDelayed");
+ try
+ {
+ api.Event.EnqueueMainThreadTask(TextChangedDelayed, "TextChangedDelayed");
+ }
+ catch
+ {
+ textChangeDelayed = false;
+ throw;
+ }
}And update TextChangedDelayed() with a try-finally block (see next comment).
🏁 Script executed:
#!/bin/bash
set -eux
# Find TextChangedDelayed implementation
rg -n "TextChangedDelayed" -C3 --type cs
# Find EnqueueMainThreadTask definition
rg -n "EnqueueMainThreadTask" -C3 --type csLength of output: 1488
🏁 Script executed:
#!/bin/bash
set -eux
sed -n '40,120p' VTMLEditor/GuiElements/Vanilla/GuiElementTextArea.csLength of output: 2082
Guard against exceptions in TextChanged workflows
- Wrap
api.Event.EnqueueMainThreadTask(TextChangedDelayed, "TextChangedDelayed")in a try/catch that resetstextChangeDelayedon scheduling failure. - Encapsulate the entire body of
TextChangedDelayed()in a try/finally to always cleartextChangeDelayed, even if an exception occurs. - Confirm
TextChanged()is only ever invoked on the main/UI thread to prevent races ontextChangeDelayed.
🤖 Prompt for AI Agents
In VTMLEditor/GuiElements/Vanilla/GuiElementTextArea.cs around lines 40-45,
TextChanged currently sets textChangeDelayed and calls
api.Event.EnqueueMainThreadTask without handling failures and TextChangedDelayed
doesn't guarantee clearing the flag; wrap the EnqueueMainThreadTask call in a
try/catch that on any exception resets textChangeDelayed and rethrows or logs
the error, and modify TextChangedDelayed so its entire body is enclosed in
try/finally that sets textChangeDelayed = false in the finally block;
additionally add a short runtime assertion or check at the start of TextChanged
(e.g. verify current thread is the main/UI thread or throw/log) to ensure
TextChanged is only invoked on the UI thread to avoid races on
textChangeDelayed.
There was a problem hiding this comment.
I changed the order of operations (swap of boolean set and enqueue) but really if the enqueue fails the game would have long crashed (pretty vital/core method for VS)
There was a problem hiding this comment.
@InsanityGod, thank you for the clarification! I can see you've addressed the exception handling concern by:
- Swapping the order - Setting
textChangedBatchedQueuedafter the enqueue call means that ifEnqueueMainThreadTaskthrows, the flag won't be stuck in atruestate - Adding try-finally - The
TextChangedBatched()method now ensures the flag is always reset in thefinallyblock, even if exceptions occur during processing
This is a solid solution, and your point about EnqueueMainThreadTask being a core VS method makes sense - if it's failing, there are bigger problems to worry about. The current implementation looks good! 👍
…nThreadTask`, not that it really matters as you'd have far bigger issues if that fails)
1 participant
throttle text update, this was the simplest way to do it but might want to look into options that allow for larger throttles.
Summary by CodeRabbit
Bug Fixes
Performance