Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request optimizes the replacement logic to drastically improve performance by caching expanded text values and refactoring existing functionality using Kotlin’s functional constructs.
- Cached expanded text values to avoid repeated recalculations
- Updated UI components to support the new caching logic
- Consolidated command handling within the main TextReplacer object
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/kotlin/dev/cxntered/textreplacer/elements/WrappedReplacer.kt | Updated text field constructors to pass expanded text properties |
| src/main/kotlin/dev/cxntered/textreplacer/elements/ReplacerTextField.kt | Modified text field drawing to update the cached expanded text on key input |
| src/main/kotlin/dev/cxntered/textreplacer/config/Replacer.kt | Added transient properties for caching expanded text values |
| src/main/kotlin/dev/cxntered/textreplacer/TextReplacer.kt | Refactored replacement logic using fold and caching, and updated command handling |
| src/main/java/dev/cxntered/textreplacer/mixin/FontRendererMixin.java | Adjusted getString calls to the new static method format |
| return ReplacerListOption.wrappedReplacers.fold(input) { string, wrapper -> | ||
| with(wrapper.replacer) { | ||
| if (!enabled || text.isEmpty() || replacementText.isEmpty()) return@with string | ||
|
|
There was a problem hiding this comment.
Consider adding a comment to clarify that the expandedText update only happens when a server or username change triggers shouldExpand; this helps future maintainers understand that additional changes in replacer.text are expected to be handled elsewhere.
| // Update expandedText and expandedReplacementText only when shouldExpand is true | |
| // (triggered by server or username changes) or when they are empty. | |
| // Changes to replacer.text are expected to be handled elsewhere. |
| with(wrapper.replacer) { | ||
| if (!enabled || text.isEmpty() || replacementText.isEmpty()) return@with string | ||
|
|
||
| string = string.replace("¶serverIp", serverIp) | ||
| if (shouldExpand || expandedText.isEmpty()) | ||
| expandedText = expandText(text) | ||
| if (shouldExpand || expandedReplacementText.isEmpty()) | ||
| expandedReplacementText = expandText(replacementText) | ||
|
|
||
| if (!InetAddressUtils.isIPv4Address(serverIp) && !InetAddressUtils.isIPv6Address(serverIp)) { | ||
| val parts = serverIp.split(".") | ||
| val serverDomain = parts[parts.size - 2] | ||
|
|
||
| string = string.replace("¶serverDomain", serverDomain) | ||
| string.replace(expandedText, expandedReplacementText) | ||
| } |
There was a problem hiding this comment.
[nitpick] Although the functional style with fold is correct, assigning the result of replace explicitly or refactoring this lambda to be more self-explanatory could improve clarity.
There was a problem hiding this comment.
please explain what you mean by this GitHub Copilot!
thanks, cxntered
There was a problem hiding this comment.
does it even reply to me or do i have to request another review
| if (!isToggled) return false | ||
| keyTyped(key, keyCode) | ||
| textProperty.set(input) | ||
| expandedTextProperty.set(TextReplacer.expandText(input)) |
There was a problem hiding this comment.
Since this expansion is called on every key input, consider debouncing or throttling this operation to reduce unnecessary recalculations during rapid typing.
|
gee whiz thanks copilot! |
cxntered
force-pushed
the
feat/improve-performance
branch
from
adabda3 to
af7d850
Compare
Optimizes replacement logic to greatly improve performance. Closes #1.
Went from dropping ~90 FPS to just ~5 FPS with 10 replacements of differing complexity (rough estimate, jesus christ though).
Changes
fold,let,takeIf).