Merged
Conversation
📝 WalkthroughWalkthroughAdds state-aware highlight colors for container buttons, implements a line-number gutter with pixel-based scrolling and gutter-aware rendering/input in the editor, increments spacing and horizontal offsets for many GUI fields/elements, adjusts checkbox/button hover and screen background/overlay render paths, and adds copy/Vec2 constructors for Pos2D/Pos3D. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as EditorScreen
participant Cursor
participant Render as RenderEngine
participant Dropdown as AutocompleteDropdown
User->>Editor: scroll / click / type
Editor->>Editor: compute contentHeight, viewportHeight, maxScroll (pixel-based)
Editor->>Cursor: update cursor position (line, col) and requested pixel position
Editor->>Editor: scrollToCursor() (center using pixel scroll & lineSpread)
Editor->>Render: set scissor(region excluding gutter) and render content at offset (lineNumWidth)
Render->>Dropdown: position dropdown at (x + lineNumWidth, y) and render if visible
Render-->>User: updated frame (gutter + content)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
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)
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java (1)
738-739: Wheel-scroll comment no longer matches implementation.Line [738] says “~40px per click”, but
SCROLL_WHEEL_SENSITIVITYis 50.0. Update the comment to prevent future confusion during tuning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java` around lines 738 - 739, Update the inline comment above the viewportHeight calculation to reflect the actual scroll wheel tuning: replace “~40px per click” with the correct value that matches SCROLL_WHEEL_SENSITIVITY (50.0) so the comment aligns with the implemented behavior; locate the comment near the viewportHeight calculation and reference SCROLL_WHEEL_SENSITIVITY and the viewportHeight computation in your update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java`:
- Around line 219-224: The computed lastLine can exceed the actual content for
short files causing lineNumWidth to be too large; clamp lastLine to the real
final line index before measuring width. After computing lastLine from
scroll/viewportHeight, set lastLine = Math.min(lastLine, <actualLastIndex>)
where <actualLastIndex> is the actual last line index for the document (e.g.
totalLines - 1 or getLineCount() - 1), then compute lineNumWidth using
font.width(...). Update the EditorScreen variables (firstLine, lastLine,
lineSpread, viewportHeight, lineNumWidth) accordingly so the gutter width
reflects content, not the viewport.
- Around line 803-806: In EditorScreen where text is trimmed (uses contentWidth,
ellipsesWidth, minecraft.font.substrByWidth,
Language.getInstance().getVisualOrder, and FormattedCharSequence.composite),
guard the computed trim width so it cannot be non-positive: compute int
trimWidth = contentWidth - 10 - ellipsesWidth, then if (trimWidth <= 0) return
ellipses (or the ellipses FormattedCharSequence) immediately; otherwise call
substrByWidth(trimWidth) and proceed with getVisualOrder and composite. This
prevents passing non-positive widths to substrByWidth and avoids unstable
trimming on narrow viewports.
---
Nitpick comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java`:
- Around line 738-739: Update the inline comment above the viewportHeight
calculation to reflect the actual scroll wheel tuning: replace “~40px per click”
with the correct value that matches SCROLL_WHEEL_SENSITIVITY (50.0) so the
comment aligns with the implemented behavior; locate the comment near the
viewportHeight calculation and reference SCROLL_WHEEL_SENSITIVITY and the
viewportHeight computation in your update.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/MacroContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/ServiceContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/CategoryTreeContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/BooleanField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/DoubleField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/FileField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/FloatField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/IntField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/LongField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/OptionsField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/StringField.javacommon/src/main/java/com/jsmacrosce/wagyourgui/elements/AnnotatedCheckBox.javacommon/src/main/java/com/jsmacrosce/wagyourgui/elements/Button.java
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.java`:
- Around line 229-231: MacroScreen.render currently calls
overlay.render(drawContext, mouseX, mouseY, delta) but skips the background;
update MacroScreen.render to either call super.render(drawContext, mouseX,
mouseY, delta) at the start or explicitly invoke
this.renderBackground(drawContext) before calling overlay.render to restore the
BaseScreen background behavior and ensure the overlay renders on top.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/CancelScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.javacommon/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.java
Show resolved
Hide resolved
…s, events, and services
Signed-off-by: Jack Manning <eastarctica@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java (2)
809-812:⚠️ Potential issue | 🟡 MinorGuard against negative trim width.
On narrow viewports,
contentWidth - 10 - ellipsesWidthcan become zero or negative, potentially causing undefined behavior insubstrByWidth.💡 Proposed fix
private FormattedCharSequence trim(Component text) { assert minecraft != null; int contentWidth = width - 10 - (lineNumWidth + 2); // total minus scrollbar minus gutter - if (minecraft.font.width(text) > contentWidth) { - FormattedCharSequence trimmed = Language.getInstance().getVisualOrder(minecraft.font.substrByWidth(text, contentWidth - 10 - ellipsesWidth)); + int trimWidth = contentWidth - 10 - ellipsesWidth; + if (minecraft.font.width(text) > contentWidth && trimWidth > 0) { + FormattedCharSequence trimmed = Language.getInstance().getVisualOrder(minecraft.font.substrByWidth(text, trimWidth)); return FormattedCharSequence.composite(trimmed, ellipses); + } else if (trimWidth <= 0 && contentWidth < minecraft.font.width(text)) { + return ellipses; } else { return text.getVisualOrderText(); } },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java` around lines 809 - 812, The trimming logic in EditorScreen can call minecraft.font.substrByWidth with a non-positive width; clamp the computed trim width used in minecraft.font.substrByWidth (contentWidth - 10 - ellipsesWidth) to a safe positive value (e.g., Math.max(0, ...)) and handle the zero/negative case by returning only the ellipses FormattedCharSequence instead of attempting to trim; update the block around variables contentWidth, ellipsesWidth and the call to minecraft.font.substrByWidth accordingly so substrByWidth never receives a negative width.
219-224:⚠️ Potential issue | 🟡 MinorClamp
lastLineto actual content line count.The calculation of
lastLinecan exceed the real last line index when the viewport is taller than the content. This causeslineNumWidthto be computed based on a line number that doesn't exist, making the gutter unnecessarily wide for short files.💡 Proposed fix
firstLine = scroll / lineSpread; // floor: include partially-visible top line lastLine = (int) Math.ceil((scroll + viewportHeight) / lineSpread) - 1; // include partially-visible bottom line + lastLine = Math.min(lastLine, totalLines - 1); // Gutter width: measure the widest line number visible (lastLine+1, 1-indexed) plus 4px padding. if (lineSpread > 0) { lineNumWidth = font.width(Component.literal((lastLine + 1) + ".").withStyle(defaultStyle)) + 4; },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java` around lines 219 - 224, Clamp the computed lastLine to the actual document line count before using it to compute gutter width: after calculating lastLine from scroll/viewportHeight, set lastLine = Math.min(lastLine, <actual total lines> - 1) (where <actual total lines> is your document line count e.g., lines.size() or totalLines) and ensure it’s not negative; then compute lineNumWidth from the clamped lastLine using font.width(...). This prevents using a non-existent line number when computing lineNumWidth in EditorScreen (variables: firstLine, lastLine, lineSpread, viewportHeight, lineNumWidth; methods: font.width(...), defaultStyle).
🧹 Nitpick comments (1)
common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.java (1)
21-23: Note: Subclass slicing when called withPos3D.Due to polymorphism, this constructor accepts
Pos3Dinstances (sincePos3D extends Pos2D), but onlyxandyare copied—thezcoordinate is silently dropped. This may be intentional (2D projection), but could surprise callers.Since
Pos3Dhas its own copy constructor that preserves all coordinates, this is likely fine. However, if unintended slicing is a concern, consider making the parameter type more explicit via documentation or a factory method pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.java` around lines 21 - 23, The Pos2D(Pos2D pos) constructor silently drops a z coordinate when passed a Pos3D (subclass) causing slicing; update the code by adding a clear Javadoc to Pos2D(Pos2D) stating it intentionally copies only x/y (2D projection) and that Pos3D should use its own copy constructor, and if you want to prevent accidental slicing instead implement one of two options: add an explicit overload Pos2D(Pos3D pos3d) that documents/handles projection or change Pos2D(Pos2D) to check `if (pos instanceof Pos3D)` and throw IllegalArgumentException (or log/warn) to avoid silent loss—refer to the Pos2D constructor and the Pos3D class when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.java`:
- Around line 809-812: The trimming logic in EditorScreen can call
minecraft.font.substrByWidth with a non-positive width; clamp the computed trim
width used in minecraft.font.substrByWidth (contentWidth - 10 - ellipsesWidth)
to a safe positive value (e.g., Math.max(0, ...)) and handle the zero/negative
case by returning only the ellipses FormattedCharSequence instead of attempting
to trim; update the block around variables contentWidth, ellipsesWidth and the
call to minecraft.font.substrByWidth accordingly so substrByWidth never receives
a negative width.
- Around line 219-224: Clamp the computed lastLine to the actual document line
count before using it to compute gutter width: after calculating lastLine from
scroll/viewportHeight, set lastLine = Math.min(lastLine, <actual total lines> -
1) (where <actual total lines> is your document line count e.g., lines.size() or
totalLines) and ensure it’s not negative; then compute lineNumWidth from the
clamped lastLine using font.width(...). This prevents using a non-existent line
number when computing lineNumWidth in EditorScreen (variables: firstLine,
lastLine, lineSpread, viewportHeight, lineNumWidth; methods: font.width(...),
defaultStyle).
---
Nitpick comments:
In `@common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.java`:
- Around line 21-23: The Pos2D(Pos2D pos) constructor silently drops a z
coordinate when passed a Pos3D (subclass) causing slicing; update the code by
adding a clear Javadoc to Pos2D(Pos2D) stating it intentionally copies only x/y
(2D projection) and that Pos3D should use its own copy constructor, and if you
want to prevent accidental slicing instead implement one of two options: add an
explicit overload Pos2D(Pos3D pos3d) that documents/handles projection or change
Pos2D(Pos2D) to check `if (pos instanceof Pos3D)` and throw
IllegalArgumentException (or log/warn) to avoid silent loss—refer to the Pos2D
constructor and the Pos3D class when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d67543c-1cc6-49c5-a031-2119e5e7cd2d
📒 Files selected for processing (5)
common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.javacommon/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos3D.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.javacommon/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java (1)
196-203:⚠️ Potential issue | 🔴 CriticalCancelScreen will lose background rendering in versions > 1.21.5 — this change needs to be revised.
While
ScriptScreenhandles its own background withrenderMenuBackground()before callingsuper.render(),CancelScreencallssuper.render()without any background handling. After this change,CancelScreenwill have no background in versions > 1.21.5, breaking its visual rendering. Either:
- Add version-gated background rendering to
CancelScreen.render(), or- Keep
renderBackground()inBaseScreen.render()for all versions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java` around lines 196 - 203, BaseScreen.render removed renderBackground causing CancelScreen to lose its background; fix by restoring background rendering for CancelScreen: update CancelScreen.render to call renderMenuBackground() or renderBackground() before calling super.render(...) for versions > 1.21.5 (i.e., add a version-gated call to renderMenuBackground()/renderBackground() at the top of CancelScreen.render), or alternatively revert and re-enable renderBackground(...) in BaseScreen.render so all subclasses keep the background; locate the relevant methods by their names BaseScreen.render, CancelScreen.render, renderBackground, and renderMenuBackground and implement one of these two fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@common/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.java`:
- Around line 196-203: BaseScreen.render removed renderBackground causing
CancelScreen to lose its background; fix by restoring background rendering for
CancelScreen: update CancelScreen.render to call renderMenuBackground() or
renderBackground() before calling super.render(...) for versions > 1.21.5 (i.e.,
add a version-gated call to renderMenuBackground()/renderBackground() at the top
of CancelScreen.render), or alternatively revert and re-enable
renderBackground(...) in BaseScreen.render so all subclasses keep the
background; locate the relevant methods by their names BaseScreen.render,
CancelScreen.render, renderBackground, and renderMenuBackground and implement
one of these two fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f0ffd50-3a02-4ddd-a643-952cf518e703
📒 Files selected for processing (19)
common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.javacommon/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos3D.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/MacroContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/ServiceContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/CancelScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/EditorScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/CategoryTreeContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/BooleanField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/DoubleField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/FileField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/FloatField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/IntField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/LongField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/OptionsField.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/StringField.javacommon/src/main/java/com/jsmacrosce/wagyourgui/BaseScreen.javacommon/src/main/java/com/jsmacrosce/wagyourgui/elements/AnnotatedCheckBox.javacommon/src/main/java/com/jsmacrosce/wagyourgui/elements/Button.java
🚧 Files skipped from review as they are similar to previous changes (12)
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/StringField.java
- common/src/main/java/com/jsmacrosce/wagyourgui/elements/Button.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/LongField.java
- common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos2D.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/IntField.java
- common/src/main/java/com/jsmacrosce/wagyourgui/elements/AnnotatedCheckBox.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/FileField.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/MacroScreen.java
- common/src/main/java/com/jsmacrosce/jsmacros/api/math/Pos3D.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/settings/settingfields/DoubleField.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/ServiceContainer.java
- common/src/main/java/com/jsmacrosce/jsmacros/client/gui/screens/CancelScreen.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9
Summary by CodeRabbit
New Features
Bug Fixes
Improvements