feat(dev_otui): visual .otui screen editor inside the client - #1782
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds a complete visual ChangesOTUI Editor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant EditorUI
participant dev_otui
participant Otml
participant OTUIFile
User->>EditorUI: Open and select OTUI file
EditorUI->>dev_otui: loadTarget()
dev_otui->>OTUIFile: read source
dev_otui->>Otml: parse source
Otml-->>dev_otui: document and widget nodes
dev_otui-->>EditorUI: render preview and properties
User->>EditorUI: Edit and apply property
EditorUI->>dev_otui: saveFile()
dev_otui->>Otml: serialize edited document
dev_otui->>OTUIFile: backup, write, and verify
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
1130253 to
ae3a042
Compare
A development tool to build and edit .otui screens while seeing the real result: the preview is drawn by OTClient's own renderer, so bitmap fonts, image-border 9-slice and the style cascade are exact, not approximated. New self-contained module; no existing file is modified. dev_otui.lua editor, inspector, drag/resize, saving otml.lua line-oriented OTML parser/writer dev_otui.otui main window (resizable) dev_otui_picker.otui file/element/anchor picker, with live preview dev_otui_gallery.otui gallery of every style in data/styles dev_otui.otmod module manifest README.md usage and design notes Features: - live preview of any .otui with hot-reload on save (300 ms), keeping the selected widget across reloads - widget tree, click-to-select on screen, selection highlight - property panel mirroring the file's LINES rather than the widget's getters, which is what exposes anchors.* and @OnClick (no getter on UIWidget) and shows every value exactly as written - dragging moves the widget through margins, honouring the anchors declared in the file; the corner handle resizes - add elements from a palette that previews each style live; new widgets are written with anchors, since an unanchored widget cannot be positioned - anchor picker offering only same-axis combinations over the parent, prev, next and every sibling with an id - guides drawing the parent's padding area and centre lines, and boxing the sibling an anchor points at - delete the selected widget from the file, children included, with a prompt - style gallery rendering all 225 styles of data/styles in one scrollable list, grouped by file; a style that cannot be sampled is listed with the reason instead of being built, which doubles as an audit of data/styles. The reason is worked out from the files before instantiating (base class not registered, style nested inside a style, image-source absent from the data folder), so the audit does not itself fill the log with engine errors. UIStatsBar, UIPopupMenu and UIPopupScrollMenu are never instantiated: they build their own children in create() and, standing alone, their layout reschedules itself forever, which freezes the client with nothing logged. The list is built one sample per frame so the window stays usable while it fills, rather than the client going dark through one 250-sample layout pass - saving back to the .otui preserves comments and formatting, writes a .bak and confirms by reading the file back after writing - style showcase for the 56 files that only define styles (every one under /styles): loadUI returns nil for them since they have no main widget, so each style is instantiated as a live sample instead - create a new screen from scratch - selfTest() covering the OTML parser, the part able to corrupt files Usage: button on the top-right menu, or Ctrl+Alt+U. Validation: modules.dev_otui.selfTest() in the terminal (Ctrl+T). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
modules/dev_otui/dev_otui_picker.otui (1)
40-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: these rows are labels, not text edits.
OtuiPickerItemderives fromUILabel; the note about a row'sTextEditneeding keyboard input looks copy-pasted from another layout and is misleading here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/dev_otui/dev_otui_picker.otui` around lines 40 - 41, Update the focusability comment in the OtuiPickerItem layout to describe these rows as UILabel-based labels, removing the incorrect reference to a row TextEdit and keyboard input while preserving any accurate focus-chain guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/dev_otui/dev_otui_picker.otui`:
- Around line 22-23: Declare an explicit style dependency from the
dev_otui_picker module to dev_otui in dev_otui_picker.otmod, ensuring
dev_otui.otui loads before dev_otui_picker and OtuiSectionLabel is available
when the picker is created or presented.
In `@modules/dev_otui/dev_otui.lua`:
- Around line 738-790: Validate each node returned by resolveRef in the
save-edit loop against currentNode’s tag and, when available, id before applying
edits; abort and reload if it differs. In the insertion path around resolveRef,
remove the doc.root fallback so unresolved references fail, and reject
currentRef.kind == 'style' parents before inserting.
- Around line 330-346: Update readDocument’s read-failure branch to call status
with a clear user-visible message, including the path or underlying read error
when available, before clearing doc and returning false. Keep the existing
parse-error reporting and successful-read behavior unchanged.
- Around line 1707-1709: Update the fallback in the skeleton-generation flow
around Otml.skeleton to use an English identifier, and ensure sanitizing the
filename cannot leave id empty by applying the same fallback after gsub.
Preserve the existing filename-derived identifier when it contains alphanumeric
characters.
- Around line 811-831: Update closeStage to reset nodeError alongside
currentNode and currentRef, ensuring post-close operations such as saveFile and
addElement cannot reuse a stale error message.
- Around line 1034-1048: Update beginDrag to return immediately when
selectedWidget is nil or invalid before calling getRect or accessing its margin
methods, while preserving the existing drag initialization for valid widgets.
In `@modules/dev_otui/README.md`:
- Line 36: Update the state-toggle documentation near the
`$on`/`$checked`/`$disabled` entry to include the `$hidden` toggle implemented
by `applyState` and `refreshStateChecks` via `stateHidden`; also hyphenate
“self-test” at the referenced prose location for consistency.
---
Nitpick comments:
In `@modules/dev_otui/dev_otui_picker.otui`:
- Around line 40-41: Update the focusability comment in the OtuiPickerItem
layout to describe these rows as UILabel-based labels, removing the incorrect
reference to a row TextEdit and keyboard input while preserving any accurate
focus-chain guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97f82932-4c1f-496e-89d8-38713002d7d5
📒 Files selected for processing (6)
modules/dev_otui/README.mdmodules/dev_otui/dev_otui.luamodules/dev_otui/dev_otui.otmodmodules/dev_otui/dev_otui.otuimodules/dev_otui/dev_otui_picker.otuimodules/dev_otui/otml.lua
readDocument cleared the document and returned false without saying why. Callers ignore the return value, so a read error surfaced only later, as the vaguer "file was not read" on the next selection. selfTest covers it: reading a missing path must return false, clear the document and leave the reason on the status line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… widget
A reference into the file is a path of child indices. It survives a reparse,
but it says nothing about which widget sits there now: a file reordered in a
text editor between load and save resolves the same path onto a different
widget, and the edit lands on it.
Both write paths now check identity (style name + id) before touching
anything:
- saveFile aborts when the re-read document no longer has the selected
widget at that path
- insertElement no longer falls back to doc.root when the reference fails
to resolve, which quietly inserted the new element somewhere else
selfTest covers it with a reordered file, a renamed widget and an unresolved
reference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
closeStage reset currentNode and currentRef but kept nodeError, so a save or an insert attempted afterwards reported why the *previous* widget could not be mapped. The three are now cleared together through clearNodeSelection, since nodeError only means anything as the reason currentNode is missing. selfTest covers it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The click handler calls beginDrag right after selectWidget, and selectWidget returns early - leaving the selection untouched, possibly nil - when it is re-entered or the widget was already destroyed. beginDrag then indexed nil inside the mouse handler. It now declines and returns false instead. selfTest covers it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…empty
The fallback id for a new screen was 'minhaJanela', Portuguese in a module
that is otherwise English. Stripping everything that is not a letter or a
digit could also leave nothing at all ("__.otui"), and a widget written with
an empty id cannot be looked up by name afterwards.
Both are now handled in screenIdFromPath, which falls back to 'newScreen'.
The self test used Portuguese names for the same reason and is now English.
selfTest covers the punctuation, the already-valid name, the name with no
alphanumerics and the empty path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The controls table listed $on, $checked and $disabled but not the fourth toggle, which applyState and refreshStateChecks implement as 'hidden' (setVisible). Also hyphenates "self-test" to match the surrounding prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
189b4c8 to
657bc94
Compare
|
Went through the review one comment at a time. Six of the seven were valid and are fixed, one commit each, every one covered by
The one I did not act on is the picker style dependency:
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modules/dev_otui/dev_otui.lua (1)
688-711: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCulprit search iterates
pairs, so the reported property depends on hash order.When several properties are invalid, the reported one is arbitrary, and each retry leaves partially merged styles on the widget. Iterating
propRowsorder (and stopping there) makes the message deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/dev_otui/dev_otui.lua` around lines 688 - 711, Update the culprit search after the bulk merge in the selectedWidget style-preview flow to iterate properties in propRows order instead of pairs(style), stopping at the first failing property. Use the corresponding property key and value from each propRows entry, while preserving the existing culprit hint and status messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/dev_otui/dev_otui.lua`:
- Around line 1546-1580: Update insertElement to reject style-definition
selections before resolving or writing the parent: when parentRef.kind is
"style", show the existing error status and return without modifying the
document. Keep widget selections resolving through resolveRef and preserve the
root fallback only when no parentRef exists.
- Around line 1663-1686: Update deleteSelectedNode to validate the resolved node
with sameNodeIdentity, matching saveFile’s identity check, before calling
Otml.removeNode. If the identity no longer matches after
readDocument(targetPath), report the stale or changed selection and return
without deleting; preserve the existing removal flow for matching nodes.
---
Nitpick comments:
In `@modules/dev_otui/dev_otui.lua`:
- Around line 688-711: Update the culprit search after the bulk merge in the
selectedWidget style-preview flow to iterate properties in propRows order
instead of pairs(style), stopping at the first failing property. Use the
corresponding property key and value from each propRows entry, while preserving
the existing culprit hint and status messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cdafc70-824d-40bc-b728-7b42e106aa19
📒 Files selected for processing (7)
modules/dev_otui/README.mdmodules/dev_otui/dev_otui.luamodules/dev_otui/dev_otui.otmodmodules/dev_otui/dev_otui.otuimodules/dev_otui/dev_otui_gallery.otuimodules/dev_otui/dev_otui_picker.otuimodules/dev_otui/otml.lua
🚧 Files skipped from review as they are similar to previous changes (5)
- modules/dev_otui/dev_otui.otmod
- modules/dev_otui/dev_otui.otui
- modules/dev_otui/otml.lua
- modules/dev_otui/dev_otui_picker.otui
- modules/dev_otui/README.md
Description
Adds
modules/dev_otui, a development tool for building and editing.otuiscreens while seeing the real result. The preview is drawn by OTClient's own renderer, so bitmap fonts,image-border9-slice and the whole style cascade are exact rather than an approximation.Motivation. Authoring
.otuitoday is a blind loop: edit the file, restart the client or reload the module, look at it, repeat. Anchors, widget states ($hover,$pressed,$on) and style inheritance are hard to reason about without seeing them applied.Design decision worth reviewing. The property panel mirrors the lines of the file (through a small OTML parser in
otml.lua), not the widget's getters. That is what makesanchors.*and@onClickeditable — they have no getter onUIWidget— and it is why saving preserves comments, blank lines and formatting: only the changed lines are rewritten, never the whole file.Scope. 7 new files, no existing file modified. Only dependency is
client_topmenu(for the toolbar button). Deleting the folder fully removes the feature.dev_otui.luaotml.luadev_otui.otuidev_otui_picker.otuidev_otui_gallery.otuidev_otui.otmodREADME.mdFeatures
.otuiwith hot-reload on save (300 ms), keeping the selected widget across reloads.Anchor...offers only legal combinations — an edge hooks only to an edge of the same axis — over the parent,prev,nextand every sibling that has an id.prev.bottomstops being guesswork..otuiwith a.bakbackup, verified by re-reading the file after writing..otuifiles (every file under/styles) define only styles and have no main widget, soloadUIreturnsnilfor them. For those, the module instantiates a live sample of each style so they can be inspected and edited too.data/stylesrendered in one scrollable list, grouped by file, and doubling as an audit of it. A style that cannot be sampled is listed with the reason rather than hidden, and the reason is worked out from the files before instantiating (base class not registered, a style nested inside another style, animage-sourceabsent from the data folder), so the audit does not itself fill the log with engine errors. Three classes are refused up front —UIStatsBar,UIPopupMenu,UIPopupScrollMenu— because they build their own children increate()and, with no host to sit in, their layout keeps rescheduling itself; since deferred events run inside the same dispatcher poll, that freezes the client with nothing logged. The list is built one sample per frame, so the window is drawn and usable while it fills.selfTest()covering the OTML parser.Usage. Button on the top-right menu, or
Ctrl+Alt+U. Seemodules/dev_otui/README.md.Behavior
Actual
There is no way to see an
.otuiscreen while writing it. Every change requires reloading the module or restarting the client, and files under/stylescannot be visualised at all — they have no main widget, soloadUIreturnsniland nothing is drawn.Expected
The screen renders live in the client while being edited, with a widget tree, editable properties, drag-to-move, and saving back to the file preserving its formatting. Style-only files render as a showcase of their styles.
Fixes
N/A — no existing issue; this is a new development tool.
Type of change
Non-breaking: the module is self-contained and does not modify or hook into any existing module.
How Has This Been Tested
Test A — OTML parser (automated). In the client terminal (
Ctrl+T):Runs 60 assertions, most of them against the parser, which is the part capable of corrupting files. Covers: byte-for-byte round-trip (parse + serialise must return the original file unchanged), composite blocks such as
layout:not being mistaken for child widgets, editing a property without touching neighbouring lines, inserting a property at the correct indentation, removing a composite block together with its children, inserting child widgets with anchors at the right indentation, removing a widget together with its children without touching siblings, and style-only files. It also covers the guards added in review: a file reference resolving onto a different widget after the file is reordered, a read failure reaching the user, the selection state being cleared as a unit, a drag refusing to start with no selection, and the id derived from a new screen's file name. PrintsselfTest: OKon success.Test B — CI checks reproduced locally. The same tools the workflows run, executed against this module in a container:
luajitcompiles both files with no error.luacheckreports 261 warnings / 0 errors, all of themaccessing undefined variablefor OTClient globals (g_ui,modules,Keybind), which every module in the project produces since there is no.luacheckrc. Warning density is comparable to or lower than existing modules (0.24 per line ingame_battle/battle.lua, 0.34 ingame_containers/containers.lua). No unused variables, shadowing or empty branches.The
.otuifiles were additionally checked for circular sibling anchors, which the layout engine rejects at runtime — none present.Test C — full edit cycle (manual).
Ctrl+Alt+U, thenOpen...and pick any.otui(e.g./game_console/console.otui) — the screen renders and the widget tree is populated.Apply (preview)— the change shows immediately without touching the file.Save to file— a.bakis written next to the file, and the tool re-reads the file to confirm the content before reporting success.git diffshould show only the edited lines./styles/10-buttons.otui— every button style renders as a live sample.Test D — style gallery (manual, run on a real client).
Style galleryrenders every style indata/stylesand reports the ones it refuses to build. Three defects were found and fixed through it while testing:UIPopupScrollMenufroze the client outright — no error, no log line, nothing drawn. Itscreate()anchors a scroll area and a scroll bar to itself, and standing alone that arrangement never settles: each layout pass schedules another throughUILayout::updateLater, and deferred events are processed inside the sameEventDispatcher::poll, so the frame never ends.UIStatsBarandUIPopupMenuhave the same shape and are refused with it.Test Configuration:
main. The toolbar icon istopbuttons/buttons, unused by any other module, so it does not duplicate an existing onegetRealDir/getWorkDirand deserves a check on another OS)Checklist
luacheckclean of real findingsTextEditto receive input (recursiveFocusonly focuses focusable levels, andpropagateOnKeyTextonly descends into focused children), and why sibling anchors must never be mutualmodules/dev_otui/README.mdcovers usage, design and safetyluajit -bcompiles clean andluacheckreports 0 errorsselfTest(), see Test A