Open
Conversation
…s, events, and services
📝 WalkthroughWalkthroughUpdates add state-aware highlight colors for container buttons, refactor EditorScreen to support a line-number gutter with pixel-based scrolling and adjusted input/render offsets, and make small spacing/offset tweaks across multiple GUI elements and fields. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as EditorScreen
participant Cursor as CursorManager
participant Render as RenderEngine
participant Dropdown as AutocompleteDropdown
User->>Editor: mouse scroll / click / key (move cursor)
Editor->>Editor: compute contentHeight, viewportHeight, maxScroll
Editor->>Cursor: update cursor position (line, column)
Editor->>Editor: scrollToCursor() → compute new scroll (pixel-based) considering lineNumWidth
Editor->>Render: set scissor(region excluding gutter) and render text/content at offset (lineNumWidth)
Render->>Dropdown: position dropdown at (x + lineNumWidth, y) and render if visible
Render-->>User: updated frame (gutter, content, highlights)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
Comment on lines
+219
to
224
| firstLine = scroll / lineSpread; // floor: include partially-visible top line | ||
| lastLine = (int) Math.ceil((scroll + viewportHeight) / lineSpread) - 1; // include partially-visible bottom line | ||
| // 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; | ||
| } |
There was a problem hiding this comment.
Clamp gutter width to actual content line count.
On Lines [220-223], lastLine can exceed the real last line when the viewport is taller than content, which makes lineNumWidth too 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
+ firstLine = scroll / lineSpread; // floor: include partially-visible top line
+ lastLine = (int) Math.ceil((scroll + viewportHeight) / lineSpread) - 1; // include partially-visible bottom line
// 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;
+ int maxVisibleLine = Math.min(totalLines - 1, Math.max(firstLine, lastLine));
+ lineNumWidth = font.width(Component.literal((maxVisibleLine + 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, 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.
Comment on lines
+803
to
806
| 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)); | ||
| return FormattedCharSequence.composite(trimmed, ellipses); |
There was a problem hiding this comment.
Guard trim width against narrow viewport/gutter combinations.
On Lines [803-806], contentWidth - 10 - ellipsesWidth can go non-positive on narrow screens, which can produce unstable trimming behavior.
💡 Proposed fix
- 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 contentWidth = width - 10 - (lineNumWidth + 2); // total minus scrollbar minus gutter
+ int trimWidth = Math.max(0, 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 (minecraft.font.width(text) > contentWidth) {
+ 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 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.
Signed-off-by: Jack Manning <eastarctica@gmail.com>
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
Comment on lines
+229
to
+231
| if (overlay != null) { | ||
| overlay.render(drawContext, mouseX, mouseY, delta); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find BaseScreen.java location
fd -t f "BaseScreen\.java" --type f
# Search for BaseScreen render method with context
rg -n -B 3 -A 25 'public void render\(' --glob '**/BaseScreen.java'Repository: JsMacrosCE/JsMacros
Length of output: 2774
Call super.render() or add missing renderBackground() call.
The render method in BaseScreen calls this.renderBackground() before rendering the overlay. MacroScreen overrides render() but skips the background rendering entirely, calling only overlay.render(). This will result in the screen background not being rendered.
Replace lines 229-231 with super.render(drawContext, mouseX, mouseY, delta); or add the missing background rendering call explicitly.
🤖 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/MacroScreen.java`
around lines 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.
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.
Summary by CodeRabbit
New Features
Bug Fixes