52 portrait layouts per aspect - #56
Merged
Merged
Conversation
The grid was frozen at 12x7 deliberately, and the reasoning was written down: those are square cells on an 11" tablet held landscape, portrait is the same grid with taller cells, and widgets that care centre their content rather than the app carrying a layout per orientation. That trade holds for a tablet turned sideways. It does not survive a phone — 393dp of width across 12 columns is 32dp, so every span a widget declares, and ShortcutWidget's square-tile logic with them, is being measured against a 4:1 stripe. A phone did not get a cramped page, it got a wrong one. So a page now carries an arrangement per aspect, and portrait gets its own preset: 6x12, picked the same way 12x7 was, against the body a phone actually gives once #51 has taken the chrome off. An iPhone 15 Pro standing up is 393x793dp, so 377x777 after the grid's padding, which is 56x57dp per cell — square again, and a 1x1 shortcut lands at 56dp, comfortably over a thumb. Six columns rather than five or seven because it is exactly half of twelve, so rescaling an arrangement from landscape rounds no column edge. No storage-key bump, unlike the issue's plan. `portrait` is a constructor default derived from `landscape`, which kotlinx.serialization reads as an optional field: a payload written before this simply has no such key, the default fills one in, and every page a user built survives. Bumping the key would have thrown all of them away to avoid a problem that isn't there. The two arrangements share instance ids, so a tile is the same tile in both orientations and its zoom, filters and rig position follow it round instead of resetting when the device turns. That made the store's purge a live hazard: it keyed on one layout's cells, so deleting a tile in portrait would have wiped the settings of the landscape tile still sitting on the page. It unions both now, with a test on exactly that. The seeds are laid out by hand in both aspects rather than left to the derived default. The landscape Vehicle page is built around a band — front, where you are, back, read across the top — and halving its width does not give you that idea in a narrower form, it gives you a broken one. Portrait makes it a stack. SeedPageTest's invariants all run over both arrangements now, since the hand-authoring doubled and the portrait half is the one nobody looks at on a desk, and GridAspectTest pins the cell arithmetic itself: the cell size is the entire argument for these constants, so it is what should fail if they move. Widget floors are left exactly as they were. They are declared in cells and portrait cells are 62% of landscape ones, so the same floor is a physically smaller tile — engine at its 3x2 minimum is 184x122dp here against 289x188 on the tablet. Nothing is broken by that and a tile can always be made bigger, but whether those minimums want raising is a judgement to make looking at a phone, and it can't be one number: a single floor pair cannot serve two cell sizes, so it likely means per-aspect floors. Not worth inventing on spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A modern tractor puts a small display between the windscreen and the right-hand window carrying what used to live behind the wheel: engine speed, ground speed, gear, cruise, telltale lamps and a strip of levels. With a phone clamped in the cab as a second screen (#51) on a grid that fits it (#52), that is now a thing we can build — and almost all of the data was already in the model, so this is a presentation issue rather than a plumbing one. Three widgets, not one cluster widget. What makes these a cluster is the page they are stacked on, not anything they know about each other, and splitting them keeps each placeable on the tablet's pages too — a telltale band above a map is a perfectly reasonable thing to want. It also matches where #19 left things: a tile is a tile, and its configuration is per instance. The rule worth stating plainly is the telltale band's: a lamp we have no state for is **absent**, not off. An unlit diff-lock lamp is a claim about the drivetrain, and that claim is only ours to make when Enhanced Vehicle is installed and managing this vehicle — which is exactly why #50 carried those three through as nullables rather than defaulting them. A cluster that quietly lies about the machine is the class of bug nobody notices until it matters, so it is what the tests lean on hardest. ConfigOption grew multi-select for it. WidgetConfig's own note said the first widget that genuinely needed more than a single pick was the moment to widen it, and "which lamps" is that widget: what matters differs per rig, and the alternative is a band that grows forever as the model gains states. resolveAll differs from resolve at both ends — unset means every choice, because a band showing nothing until configured just looks broken, and a stale value is dropped rather than handed back, because for a set that is one missing lamp rather than a tile whose whole subject went wrong. The digits are a heavy monospace rather than a seven-segment face. A real segment font means licensing a bundled file or drawing segments by hand, and this gets most of the effect for neither; monospace matters more than the shapes anyway, since it stops the readout shuffling sideways as digits change. Seeded as a Pillar page, AutoShow.Never: this is the page you pin a second device to (?display=pillar), and one that also grabbed the tablet whenever you climbed into a cab would be fighting the Vehicle page for the screen you are actually holding. Sized against a real phone rather than guessed. The first pass floated in a half-empty panel, so the readout now takes its type size from the tile and right-aligns the numbers with their units beside them, the lamps size themselves to pack the band, and the bars are an equal share of the width capped short of becoming slabs, on a light track so a part-full bar reads. Levels are the engine's three tanks only: the strip answers "can the machine keep going", which is the same three bars on every rig, while what is in the hopper changes shape as you hitch things up and is already answered properly, with names and figures, by the rig-slot tiles. Out of scope, per the issue: iTEC, which we do not model. The reference's tractor-and-tool pictogram is RigSlotWidget's job and already exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Material has a lightbulb, a parking sign and a padlock. A telltale is not an app icon: the shapes a driver reads without thinking are the ones printed on a real lens, and borrowing something that is merely *about* the same subject costs a moment of thought on every glance — on the one instrument that exists to be read without looking at it. So the cluster draws its own now, twenty glyphs on a 24x24 viewport, fills only: a stroke's width is in the vector's own units and its caps and joins are one more thing to get right per renderer, while a filled outline scales exactly from 14dp to 48dp. Where the machine appears in a glyph it is drawn the way that lamp's subject is seen. From the side, facing left as on the Lighting panel's schematic, for the work lights throwing a beam forward or back. From above for the drivetrain, where all-wheel drive and the two diff locks are one picture: the axle a lamp is about is solid, the other is left open, and the front wheels are steered, which is the only thing in the drawing saying which end is which. Three things about the band changed with them. A lamp that is off is not drawn at all. On a real cluster the symbol is printed on the lens and exists only when it is backlit, and that is the whole value of the thing — everything you can see is true right now, rather than being the bright ones picked out of a row of grey ones. It keeps its slot while dark, though: the alternative reflows the band every time a lamp changes, which moves every other lamp out from under the eye already on it. The turn arrows are pinned to the tile's edges, and they flash. Left means left because it is on the left, out at the corner of your eye on the side you are about to turn towards; a pair of similar arrows side by side in the middle of a row is a fraction of a second spent telling them apart. Hazards are both of them at once, which is what the machine is doing, so there is no third lamp saying so. The flasher only runs while something is flashing: a pinned display is a screen awake for a whole session, and an infinite transition nobody can see is a wake-up per frame for all the time you are not indicating. Every lamp lights for 2.2s when the band appears — the bulb check a machine does at ignition, and the only time you see the band whole, which makes it the only time you can tell the tile is configured the way you meant. It checks the lamps this band *has*: one with no state behind it has no bulb to check, and lighting it would mean the band changing shape as the check ended. The reverser is not a lamp. A direction sat oddly among telltales that are all faults and settings, so it moved to the middle tile as the machine seen from above with an arrow, in place of the F/R that used to print beside the speed — which is where the 6R's own display puts it. Its slot is held open on every line whether or not anything is in it: inside the label column its coming and going dragged the unit and the digits sideways each time the machine stopped. Five glyphs are drawn ahead of their data — battery, coolant temperature, general warning, brake system, service — for a maintenance mod we do not export yet. None of them is a Telltale: a lamp with no source behind it would be a choice in the band's config that can never light. The service one is deliberately a lamp rather than the word SERVICE that mod puts over its own speedometer, which is not something a band of glyphs can carry. The Lighting panel takes the same glyphs. A panel where "work lights" is a lightbulb while the band shows a tractor throwing a beam is two names for one switch. Hand-written path data fails silently — a typo is a lamp that never comes on — so the tests parse every glyph, assert that no two of them are the same drawing, and check that every lamp in the band has one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds Pillar instrument-cluster widgets and transmission-direction telemetry. It also adds fixed landscape and portrait grids, independent page arrangements, orientation-aware editing and persistence, seeded layouts, rendering assets, configuration support, and tests. ChangesPillar cluster instrumentation
Orientation-aware page layouts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
The cluster wants to show which way the machine is pointed, and speed.direction cannot answer it. Drivable:getDrivingDirection() returns 0 below 0.2 km/h, so the moment you stop at a gateway the readout goes blank — precisely when you most want to know whether letting the brake off will take you forward or back. The obvious-looking source is a trap, and worth writing down because the name invites it. vehicle:getReverserDirection() sounds like the shuttle lever, and is not: grep for who *writes* spec_drivable.reverserDirection and the only caller is ReverseDriving.lua, the reversible driving position — the seat and console swivelled round on a machine built to work in both directions. On a tractor without that feature it is initialised from vehicle.drivable #reverserDirection, defaults to 1, and is never touched again. Exporting it gave a field that read FORWARD for ever, which is how it was caught: in the app, not in a test, because there is nothing wrong with the code. So it comes from the motor instead. VehicleMotor:getDrivingDirection() is currentDirection * transmissionDirection — what the transmission is actually in — and it is the value the engine feeds its own dashboards as Motorized's movingDirectionLetter, the F / R / N a vehicle prints on its instrument panel. Being the game's own answer is the point: we are not deriving a direction, we are reporting the one it already shows. It lives on motor rather than on speed, next to the gear, because that is what it belongs to. speed.direction stays exactly what it was — the way the machine is travelling — and the two disagreeing is now expressible, which is the whole reason for having both: stood still in gear is "we will go forwards", not "we are going forwards". STOPPED on this field means **neutral**, not "not moving". With automatic direction change the motor reports neutral below about 1 km/h, and that is what the game shows there too, so it is reported rather than smoothed away. Absent means the mod predates version 6 or the vehicle has no motor to ask — never "forward". The committed captures are all version 5 and cannot show any of this, so the decode is covered inline instead: a tractor stopped with reverse selected, neutral decoding as a real value rather than as absence, and combine.json asserting that a capture without the field stays null. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A review round on the running cluster, against the photo of the real 6R panel in references/img.png. Eight points, most of them the difference between an instrument that looks like the thing and one that merely reports the same numbers. The digits are a real seven-segment face now. The previous commit called monospace "most of the effect" for neither a licence nor hand-drawn segments; having looked at both side by side, it isn't. DSEG Classic is SIL OFL, so it ships (licences/DSEG-OFL-1.1.txt) and the shapes are simply right. Three of its conventions carry weight: `!` is an all-off cell, an all-on cell is what the faint ghost layer behind every readout is made of, and `.` has zero advance, so 12.4 is three cells and not four. Each line reserves a fixed cell count, which is better than right-alignment alone — a value getting shorter unlights a cell instead of moving the ones beside it, and the unlit segments sitting there faintly is what the panel actually looks like. Two faces, because seven segments physically cannot draw a capital R, N or D and DSEG7 substitutes half-height lowercase. Beside a full-height F that reads as a smaller letter rather than a different one. So the two fields that can hold letters — the direction and the gear — are set in the fourteen-segment face, which is the same split a real dashboard makes between its numeric and alphanumeric fields. The gear is alphanumeric throughout rather than only when its value happens to contain a letter, so the field does not change face under itself as you shift. The gear gets its own line, amber beside the cruise, both being things the driver selected rather than things the engine is reporting. The direction sits on the speed line as an arrow plus its letter — on the speed and not on the gear, because a gear line is not a given (a combine reports none) and losing the direction along with it would be a poor trade. It flashes while the machine is standing still, which is the difference between "we are going forwards" and "we will". Neutral neither flashes nor draws an arrow: N at a standstill is simply true, and there is nothing provisional to flash. The reverser glyph is gone, replaced by a plain arrow. It was the machine drawn from above with an arrow, and at 24 units there is no room for both — the wheels fused with the arrowhead into a shape that read as neither. Going back to the reference settles it: its own direction indicators are a plain up arrow and a plain down arrow, and the little plan-view chassis between them is a different symbol altogether. The level strip is redrawn after the photo: an open-topped frame, green over the working range and red across the tenth where the gauge is in trouble, with a light ten-band level standing inside it. The colour is on the frame rather than on the contents, which says where the trouble *starts* before you are in it — something a bar that only turns red on arrival cannot do, since by then there is nothing left to compare against. Coolant temperature joins the tanks and needed the strip to learn that a gauge can be in trouble at either end: a tank is bad near empty, a temperature near full. The width cap is now a share of the bar's own height rather than a fixed dp, after four bars came out stranded in the middle of a portrait tile with half the width empty. The maintenance glyphs become ordinary lamps. Coolant temperature and general warning have sources — motor.temperatur and wearable.damage — which is exactly what engineWarning used to derive from, so that lamp stops being the union of two facts and starts waiting, with battery, brake system and service, for the mod that will report an actual engine fault. It keeps its key and its glyph; the lamps with no channel behind them are permanently absent rather than permanently off, which is the same rule the drivetrain trio follows. A pinned display paints the whole viewport black. The tablet is a thing you hold under a cab roof and its light panels are right for that, but a phone clamped to the A-pillar is an instrument in the driver's eyeline, and on the terminal's grey the gutters between the black tiles lit up as a grid of seams. Two smaller things found by looking rather than reading. The label column is a fixed width now: sized to its own content, CRUISE's column was wider than RPM's, which narrowed the box beside it and walked that line's digits inwards, so the four numbers did not share the right edge this layout exists to give them. And neutral's N takes the digits' white rather than the label grey, which sat close enough to its own ghost to be barely legible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Grisu118
marked this pull request as ready for review
July 31, 2026 16:35
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/WidgetDashboard.kt (1)
73-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRotating mid-dialog can silently discard a widget reconfiguration.
addAtandconfiguringareremembered beforeaspectis known (aspectis only computed insideBoxWithConstraintsat Line 85). Neither is reset whenaspectchanges.If the user rotates the device (or resizes across the breakpoint) while a
ConfigTarget.Placeddialog is open, Line 176 callslayout.reconfigure(target.cell.instanceId, config)against the new aspect's layout.GridLayout.reconfigureno-ops when the instance id is not present in that layout. If the tile being configured is not shared with the other aspect's arrangement, the dialog closes as if the save succeeded, but the edit is silently dropped.Reset the in-flight add/configure state when the aspect changes, so a rotation cannot let a stale
ConfigTargetreach the new layout.🔧 Proposed fix: clear in-flight dialogs on aspect change
BoxWithConstraints(modifier.fillMaxWidth().weight(1f)) { val aspect = GridAspect.of(maxWidth, maxHeight) val layout = page.layoutFor(aspect) + // A rotation mid-dialog would otherwise let a stale ConfigTarget/addAt from the previous + // aspect reach reconfigure()/addWidget() against the new layout and silently no-op. + LaunchedEffect(aspect) { + addAt = null + configuring = null + } + fun apply(next: GridLayout) = pageStore.update(page.withLayout(aspect, next))Also applies to: 176-176
🤖 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 `@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/WidgetDashboard.kt` around lines 73 - 88, Reset the in-flight add/configure state whenever the computed GridAspect changes. Move or key the addAt and configuring state in the BoxWithConstraints scope using aspect (or otherwise observe aspect changes) so stale ConfigTarget.Placed values cannot reach layout.reconfigure after rotation or breakpoint changes; preserve the existing dialog behavior when the aspect remains unchanged.
🧹 Nitpick comments (2)
VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterTest.kt (1)
291-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe overflow test hardcodes
LAMP_GAP, so it can go stale.The test re-implements the packing formula with a literal
8f.LAMP_GAPis private inTelltales.kt. If the gap changes, this test keeps checking the old geometry and still passes. Expose the gap asinternaland read it here, so the assertion tracks the implementation.🤖 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 `@VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterTest.kt` around lines 291 - 300, Update the `LAMP_GAP` declaration in `Telltales.kt` from private to internal visibility, then replace both hardcoded `8f` values in `everyLampFitsTheTileItWasSizedFor` with `LAMP_GAP` so the test uses the implementation’s current gap.VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.kt (1)
60-66: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider keeping the "unset means all" default instead of materialising every choice.
For a new tile
initialis empty, soresolveAllreturns every choice and line 62 stores the full list. Every saved tile then holds a frozen snapshot of the choices that existed at save time. When a newTelltaleentry is added, existing bands never show it, even though the user never deselected anything.Omit the key when the selection covers all choices, so
resolveAllkeeps applying its documented all-by-default rule.♻️ Proposed change
for (option in options) { if (option.multi) { - put(option.key, ConfigOption.join(option.resolveAll(initial))) + // Leave the key absent while the answer is still "all of them", so a band picks up + // choices added in a later version. + if (option.key in initial) put(option.key, ConfigOption.join(option.resolveAll(initial))) } else { option.resolve(initial)?.let { put(option.key, it) } } }The toggle path then needs the same default when nothing is stored yet:
- val chosen = selection[option.key].orEmpty().split(ConfigOption.SEPARATOR).toMutableSet() + val chosen = option.resolveAll(selection.toMap()).toMutableSet()Also applies to: 101-115
🤖 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 `@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.kt` around lines 60 - 66, Update the multi-select handling in the WidgetConfigDialog option-processing loop and its corresponding toggle path so a selection resolving to all available choices leaves the configuration key unset, preserving the documented “unset means all” behavior. When no value is stored, ensure the toggle path also treats the selection as all by default rather than requiring a materialized list.
🤖 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 `@vdTelemetry/VDTelemetry.lua`:
- Around line 127-129: Add a version 6 JSON fixture under the existing
examples/json fixtures, including the new motor.direction field with the
expected value and protocol version. Update VdtModelTest to load this fixture
and assert motor.direction through the Kotlin model, keeping the fixture
consistent with VDTelemetry.VERSION and Model.kt.
In
`@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterReadout.kt`:
- Around line 326-329: Update format1 so negative scaled values retain a single
leading minus sign while formatting the absolute magnitude for both integer and
fractional parts; avoid reconstructing negatives from scaled / 10 and an
independently negated remainder, preserving the existing one-decimal output for
nonnegative values.
In
`@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/ClusterWidgets.kt`:
- Line 99: Update the KDoc for ClusterLevelsWidget to describe only the engine
tank level bars, removing the incorrect reference to the rig’s cargo bars. Keep
the documentation aligned with levelsOf and cargoIsNotOnThisStrip, which show
that cargo is excluded.
---
Outside diff comments:
In
`@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/WidgetDashboard.kt`:
- Around line 73-88: Reset the in-flight add/configure state whenever the
computed GridAspect changes. Move or key the addAt and configuring state in the
BoxWithConstraints scope using aspect (or otherwise observe aspect changes) so
stale ConfigTarget.Placed values cannot reach layout.reconfigure after rotation
or breakpoint changes; preserve the existing dialog behavior when the aspect
remains unchanged.
---
Nitpick comments:
In
`@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.kt`:
- Around line 60-66: Update the multi-select handling in the WidgetConfigDialog
option-processing loop and its corresponding toggle path so a selection
resolving to all available choices leaves the configuration key unset,
preserving the documented “unset means all” behavior. When no value is stored,
ensure the toggle path also treats the selection as all by default rather than
requiring a materialized list.
In
`@VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterTest.kt`:
- Around line 291-300: Update the `LAMP_GAP` declaration in `Telltales.kt` from
private to internal visibility, then replace both hardcoded `8f` values in
`everyLampFitsTheTileItWasSizedFor` with `LAMP_GAP` so the test uses the
implementation’s current gap.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 94e1b132-2879-4517-97b1-3350dd3b4d34
⛔ Files ignored due to path filters (3)
VDTerminal/app/src/commonMain/composeResources/font/dseg14_classic_bold.ttfis excluded by!**/*.ttfVDTerminal/app/src/commonMain/composeResources/font/dseg7_classic_bold.ttfis excluded by!**/*.ttfreferences/img.pngis excluded by!**/*.png
📒 Files selected for processing (30)
VDTerminal/README.mdVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/App.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/apps/VehicleApp.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/GridAspect.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/GridLayout.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/layout/WidgetDashboard.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/pages/Page.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/pages/PageStore.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Cluster.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterIcons.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterLevels.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterReadout.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Lighting.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Telltales.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/ClusterWidgets.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfig.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/layout/GridAspectTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/pages/PageStoreTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterIconsTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/state/DisplayStoreTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/widgets/ConfigOptionTest.ktVDTerminal/licenses/DSEG-OFL-1.1.txtVDTerminal/shared/src/commonMain/kotlin/net/vertexdezign/vdt/model/Vehicle.ktVDTerminal/shared/src/jvmTest/kotlin/net/vertexdezign/vdt/VdtModelTest.ktvdTelemetry/VDTelemetry.luavdTelemetry/src/collect/vehicle/Motor.luavdTelemetry/src/model/MotorModel.luavdTelemetry/src/model/VehicleModel.lua
Comment on lines
+127
to
+129
| -- 6: `motor.direction` — the direction the transmission is *in*, as distinct from `speed.direction`, | ||
| -- which is the way the machine is actually travelling and reads STOPPED below walking pace. | ||
| VDTelemetry.VERSION = 6 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add a version 6 JSON fixture.
This change adds a telemetry output field and bumps the protocol to version 6. The supplied changes do not update an examples/json fixture. Add a version 6 fixture that contains motor.direction, then assert it through VdtModelTest.
As per coding guidelines, “When changing telemetry JSON output, update VDTelemetry.VERSION, the Lua model, the Kotlin Model.kt, and the JSON fixtures together.”
🤖 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 `@vdTelemetry/VDTelemetry.lua` around lines 127 - 129, Add a version 6 JSON
fixture under the existing examples/json fixtures, including the new
motor.direction field with the expected value and protocol version. Update
VdtModelTest to load this fixture and assert motor.direction through the Kotlin
model, keeping the fixture consistent with VDTelemetry.VERSION and Model.kt.
Source: Coding guidelines
🔥 format1 signs off the whole value, so -0.5 no longer prints as 0.5 when integer division truncates the sign away 🔥 the level strip's doc says what levelsOf actually draws: coolant and the engine's tanks, never cargo 🔥 LAMP_GAP is internal and the packing test uses it instead of its own 8 🔥 the config dialog no longer materialises a multi option's default, so accepting the pre-ticked lamps leaves the key unset and the band keeps taking lamps added later Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An off lamp was drawn at zero alpha, so the band was empty until something happened on it. It now sits at the same GHOST_ALPHA the unlit segments do: the symbol is faintly there and *lights up*, which is what the panel this copies looks like, and lit against ghosted is a factor of ten either way, so the band is still read by what is bright on it. Two things the blank slot could not do: the band says what it is able to tell you before anything is on, and a lamp we have no state for at all is now visibly different from one that is merely off. A flasher flashes back to the ghost rather than to nothing — a lens going dark, not a symbol being taken away. Only a lit lamp is still described to a screen reader; the ghost is chrome, like the segments it matches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two lamps for two differentials cost two slots of a band that is always visible now that off lamps are ghosted, and the answer landed in whichever of two neighbours happened to light. One lamp instead: lit when either end is shut, with the glyph saying which — the axle in question drawn solid and the other left open, or both when both are. Ghosted, it draws both ends shut. That is the segment display's ghost cell convention exactly: what the thing looks like with everything lit, at an alpha nobody can mistake for lit. Telltale.iconIn is new and does nothing for the other fifteen lamps, which a test holds it to. The lamp is absent only when Enhanced Vehicle reports neither end — an end it is silent about is one we can't call shut, so a rear-only machine keeps a lamp and gets the rear glyph. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getGearGroupToDisplay returns a name *and* whether this transmission has groups at all, and the name is the placeholder "N" whenever it hasn't. We took the first return value alone, so every gearless machine claimed a group it does not have and the cluster printed it against the gear: "ND" / "NN" / "NR" on anything fully automatic, "N12" on a geared tractor with no ranges. The engine drops it on exactly this test — Motorized:getGearInfoToDisplay is `if not groupsAvailable then gearGroup = nil end` — and its own HUD only draws a group for a non-automatic transmission. Absent rather than empty, so nothing downstream has to tell an unnamed group from a missing one. The terminal needs no rule of its own now: whatever group arrives is a real range, including the "N" a ranged transmission reports with its range lever out, which is what the game shows there too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterIconsTest.kt (1)
64-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCompare glyph geometry without
ImageVector.namenoise.
ImageVector.equalsincludes the vectorname, so this assertion can pass when two glyphs duplicate the same paths but keep different identifiers. Compare canonical path geometry instead, e.g.VectorPath.pathDataplus transformations, so equal drawings fail the test.🤖 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 `@VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterIconsTest.kt` around lines 64 - 69, Update noTwoGlyphsAreTheSameDrawing to compare canonical vector path geometry rather than ImageVector equality or name-bearing objects. Derive each glyph’s comparison key from its VectorPath.pathData and any applicable transformations, then assert those geometry keys are distinct while preserving the existing duplicate-drawing failure message.vdTelemetry/src/collect/vehicle/Motor.lua (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the JSON contract across the telemetry pipeline.
VDTelemetry.VERSION,VDTelemetry.SETTINGS_XML_VERSION, the Kotlin model, and the examples/fixtures each still describe the pre-motor.directionshape, while the collector now emits version 6 with the new motor direction field.🤖 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 `@vdTelemetry/src/collect/vehicle/Motor.lua` at line 1, Update the telemetry JSON contract from the pre-motor.direction shape to version 6 across VDTelemetry.VERSION, VDTelemetry.SETTINGS_XML_VERSION, the Kotlin model, and all examples and fixtures; ensure each reflects the collector’s emitted motor.direction field and remains consistent throughout the pipeline.Source: Coding guidelines
VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Telltales.kt (1)
77-107: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd migration for the merged
diffLocklamp.
Telltales.ktexposes onlydiffLock, andConfigOption.resolveAll()drops values no longer in the lamp choices. Saved configs that selected any previous diff-lock/side lamp keys will load again with that lamp omitted instead of being restored to the mergeddiffLockentry. Migrate old diff-lock keys todiffLock, or document the upgrade as intentionally dropping them.🤖 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 `@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Telltales.kt` around lines 77 - 107, Add migration handling for legacy diff-lock and side-lamp keys so ConfigOption.resolveAll() maps any previously saved diff-lock selections to the merged Telltale.diffLock key instead of dropping them. Anchor the migration to the Telltale key definitions and the existing resolveAll() configuration flow, preserving current behavior for unrelated lamp keys.
🧹 Nitpick comments (1)
vdTelemetry/spec/Motor_spec.lua (1)
14-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
model.direction.
fakeMotormocksgetDrivingDirection, andMotor.collectnow maps it intomodel.direction. None of the four tests assert onmodel.direction. Add an assertion so a regression in the direction mapping is caught alongside the gear-group regressions this file already guards against.🤖 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 `@vdTelemetry/spec/Motor_spec.lua` around lines 14 - 59, Extend the Motor.collect assertions in each of the four tests to verify model.direction equals the mocked getDrivingDirection result from fakeMotor. Keep the existing gear and gear-group assertions unchanged and use the shared fakeMotor behavior so direction mapping regressions are covered.
🤖 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 `@vdTelemetry/src/collect/vehicle/Motor.lua`:
- Line 55: Complete the schema update for the new group and direction vehicle
fields: increment VDTelemetry.VERSION, update the Lua vehicle model around
motor:getGearGroupToDisplay(), update the corresponding Kotlin Model.kt
definitions, and revise all relevant JSON fixtures so they include and validate
the new fields consistently.
---
Outside diff comments:
In `@vdTelemetry/src/collect/vehicle/Motor.lua`:
- Line 1: Update the telemetry JSON contract from the pre-motor.direction shape
to version 6 across VDTelemetry.VERSION, VDTelemetry.SETTINGS_XML_VERSION, the
Kotlin model, and all examples and fixtures; ensure each reflects the
collector’s emitted motor.direction field and remains consistent throughout the
pipeline.
In
`@VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Telltales.kt`:
- Around line 77-107: Add migration handling for legacy diff-lock and side-lamp
keys so ConfigOption.resolveAll() maps any previously saved diff-lock selections
to the merged Telltale.diffLock key instead of dropping them. Anchor the
migration to the Telltale key definitions and the existing resolveAll()
configuration flow, preserving current behavior for unrelated lamp keys.
In
`@VDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterIconsTest.kt`:
- Around line 64-69: Update noTwoGlyphsAreTheSameDrawing to compare canonical
vector path geometry rather than ImageVector equality or name-bearing objects.
Derive each glyph’s comparison key from its VectorPath.pathData and any
applicable transformations, then assert those geometry keys are distinct while
preserving the existing duplicate-drawing failure message.
---
Nitpick comments:
In `@vdTelemetry/spec/Motor_spec.lua`:
- Around line 14-59: Extend the Motor.collect assertions in each of the four
tests to verify model.direction equals the mocked getDrivingDirection result
from fakeMotor. Keep the existing gear and gear-group assertions unchanged and
use the shared fakeMotor behavior so direction mapping regressions are covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e44a1830-e970-4658-a47a-72a13ab5dd09
📒 Files selected for processing (13)
VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Cluster.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterIcons.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/ClusterReadout.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Telltales.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/ClusterWidgets.ktVDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterIconsTest.ktVDTerminal/app/src/commonTest/kotlin/net/vertexdezign/vdt/app/panels/ClusterTest.ktVDTerminal/shared/src/commonMain/kotlin/net/vertexdezign/vdt/model/Vehicle.ktVDTerminal/shared/src/jvmTest/kotlin/net/vertexdezign/vdt/VdtModelTest.ktvdTelemetry/spec/Motor_spec.luavdTelemetry/src/collect/vehicle/Motor.luavdTelemetry/src/model/MotorModel.lua
🚧 Files skipped from review as they are similar to previous changes (4)
- VDTerminal/shared/src/jvmTest/kotlin/net/vertexdezign/vdt/VdtModelTest.kt
- VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/WidgetConfigDialog.kt
- VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/widgets/ClusterWidgets.kt
- VDTerminal/app/src/commonMain/kotlin/net/vertexdezign/vdt/app/panels/Cluster.kt
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 #52 #53
Still open:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation