Fix silent corruption, data loss and crashes in the editor UI - #2
Merged
Merged
Conversation
PokEditor builds with Maven but its .gitignore only covered the IntelliJ out/ directory, leaving target/ untracked in every working copy after a build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Silent corruption: - LearnsetsTable grew the underlying learnset from getValueFor, a read path called while painting. Scrolling the sheet appended padding entries that serialize as move 0 at level 0 rather than the 0xFFFF terminator, and the damage compounded on every reopen. The getter no longer mutates, and the setter's boundary check is corrected alongside it. - NumberOnlyCellEditor's range check was a tautology applied to the value the cell held before editing, so nothing was validated and out-of-range input was truncated to a byte on save. It now validates the typed text against a real per-column range. - The same editor's document filter stripped every non-digit including the minus sign, and setText goes through the filter -- so merely opening the editor on a negative move priority flipped its sign. - resetIndexedCellRendererText consumed the column text sources differently from loadCellRenderers, so after editing any species name the Evolutions method dropdowns listed Pokemon names and writing one stored a different evolution method. The positional queue is replaced by a column-keyed map. Data loss: - Saving never terminated an in-progress cell edit, so a typed value that had not been committed with Enter was silently discarded. - The ROM was mutated before the confirmation dialog, so declining still left the edits in memory to be flushed by a later unrelated save, and Reload restored what it was meant to discard. Preparation and commit are now separate. - The field script editor's model setter was empty, so "Script file saved!" discarded the recompiled script. - Variable-tracker substitution rewrote parameters in the shared model with names nothing could convert back, which aborted the whole ROM write. Substitution now happens only in the display path. - Level script trigger edits were made against a throwaway list model and never written back. - The arm9 write was commented out, so TM/HM move reassignments never reached the ROM. Crashes: - Importing an indexed PNG with fewer than 16 colours threw partway, leaving the palette half-updated and the sprite not imported, with no dialog. - Importing into an empty sprite slot produced a placeholder whose scan mode was unset, which saved as an all-black sprite. - Importing a party icon overwrote the battle sprite palette. - The script pane dereferenced a mouse position that is null whenever the pointer is not over the component, so Ctrl+C/V/A threw and did nothing. - Undefined labels and stale jump-list entries were used as unchecked offsets. - Shadow size was read as a full byte and pushed into a four-item combo box. - The battle mockup opened a modal dialog from inside paint, which reopened itself forever once dismissed. - Delete Row threw when the row was selected via the frozen column, and did not shift the parallel species-name bank. - Paste computed its copy count with rounding, so the commonest gesture pasted nothing and a larger selection overwrote a row below it. Honesty and completeness: - hasUnsavedChanges was hardcoded false, so closing after an hour of edits exited without a prompt. - ConsoleWindow was never instantiated, so validation failures went to a stream with no window attached. Failures are now reported next to the cell, with an uncaught-exception handler installed. - Export reported success when the chooser was cancelled. - The Find dialog had no listeners and no search logic; it is implemented. - deleteCurrentEntry was an empty override; it is implemented. - The two shiny palette buttons had no listeners and were removed rather than left looking functional. - Syntax highlighting re-lexed the whole document twice per keystroke on the EDT; it is debounced. Framework utilities: BitVector's mask set two bits instead of one, CsvReader dropped trailing empty fields and used the platform charset, ArrayModifier aborted on the first short row, Directory.delete always claimed success, and TrainerPersonalityCalculator produced a wrong TID from every seed. Updated for PokEditor-Core's API changes: parseBaseRom returns the game and region together, Tables.initialize takes the region explicitly, and the TM/HM tables are read from the parser instance rather than from statics. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Nds4j is released as 1.0.0 in the companion branch; 0.1.0 no longer exists, so this declaration has to move with it. Also updates the stale 0.1.0 reference in the WinLaF todo comment, which was waiting on exactly that Maven release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
PokEditor's version is written down twice, and the two had already drifted: pom.xml declared 3.0-SNAPSHOT while Main.java passed "3.1.1" to Tool.setVersion(). The Main.java string is the one users actually see -- it goes into the window title and the project start panel -- so the application has been reporting 3.1.1 while the build artifact called itself 3.0-SNAPSHOT. Both are now 3.2.0. The pom drops -SNAPSHOT so the coordinate and the displayed string say the same thing, which they could not while one was a development version and the other a release string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Both are cut from 1.0-SNAPSHOT in their companion branches, so these declarations move with them. A release version resolving snapshot dependencies is not reproducible. VariableTracker remains at 1.0-SNAPSHOT because it is unpublished and has no repository declaration -- see the note in the pull request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Runs on every push and pull request. None of the three sibling libraries are on Maven Central at the versions this module depends on, so each is built from source first, preferring a branch of the same name where one exists and falling back to main. Order matters: Nds4j underpins the other two. This workflow will fail until VariableTracker is resolvable. That is the intended behaviour rather than an oversight -- the repository genuinely does not build from a clean checkout, and CI reporting that honestly is the point of adding it. Dependency resolution is checked as its own named step so the cause is attributed directly, with an error annotation explaining the three ways to fix it, instead of surfacing later as an unexplained compilation failure. There is no test suite in this module; the logic it drives is covered in PokEditor-Core and Nds4j, which have their own workflows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
This module had no test infrastructure at all, so nothing in the editor frontend could be verified. Adds junit-jupiter and assertj at test scope, matching the versions already used by Nds4j, Nds4j-ToolUI and PokEditor-Core, so all four repositories share one test stack. No tests yet -- this is the enabling change on its own so that the suite that follows is a reviewable diff of tests rather than tests plus build configuration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Each of these was surfaced by a test rooted in a stated property rather than
in current behaviour, and each was confirmed against the source before being
changed.
Paint-path totality - a single bad cell used to take the whole sheet down:
- IndexedStringCellRenderer parsed a String cell with an unguarded
Integer.parseInt, so an empty cell or a partial edit threw
NumberFormatException from inside paint(). The Integer branch beside it
was already bounds-checked, so the range was considered and the
parse was not.
- CheckBoxRenderer and CheckBoxEditor cast to (Boolean) bare: null threw
NPE and any other type threw ClassCastException.
Cells that could be displayed but never corrected:
- ComboBoxCellEditor called setSelectedIndex with an unbounded value. The
matching renderer ignores an out-of-range index and paints harmlessly,
so a cell holding a bad value rendered fine but threw on the EDT when
double-clicked - the one action that could have fixed it.
Renderer and editor disagreeing about the same bitfield:
- Both derived a bit index from Math.log(val)/Math.log(2) and rounded it
at different points: (int)(log + 1) against (int)log + 1. For a negative
value Math.log returns NaN, which casts to 0, so the sheet painted one
flag while the editor preselected another - and closing the editor wrote
that second flag back to the file. Both now share one highestSetBit()
built on Integer.numberOfLeadingZeros: exact integer arithmetic, no NaN,
no last-bit rounding, and one implementation so they cannot drift again.
getCellEditorValue also guards the new -1 "nothing selected" state, which
would otherwise have evaluated 1 << -2.
Script editor:
- The ScriptDocument constructor inserted a leftover JButton("test") into
the pane.
- insertString and remove scheduled a debounced re-highlight but left the
old element ranges in place, so for 250ms the pane answered hovers and
ctrl-clicks from offsets that no longer existed; after a delete, find(0)
returned a 13-character range over a 2-character document. Every caller
of find() already handles null, so clearing is safe.
- ElementRange.toString read maxExclusive - min + 1 characters of a
half-open range, so it never returned its own text. It only avoided
throwing at end-of-document because AbstractDocument carries an implicit
trailing newline to absorb the overrun.
- ScriptElementList.add inserted once per enclosing range instead of once,
so the list grew with nesting depth and find() could return a duplicate.
- Malformed input made ANTLR error recovery match a rule against zero
tokens, producing a zero-width ElementRange whose unchecked throw escaped
the timer's BadLocationException handler onto the EDT. A syntax
highlighter must not crash the editor over syntax that is merely
half-typed. The visitors should also skip degenerate contexts; that is a
wider change across 11 construction sites and is left for review.
Dead but loaded framework code:
- Directory.delete() recursed infinitely. clearDirectory ended with
directory.delete(), and for the top-level call `directory` IS the
Directory, so it dispatched back into the override. Subdirectories arrive
as plain File from listFiles(), so only the outermost call was affected -
which is to say every call.
- BitVector accepted out-of-range indices silently. Java truncates integer
division toward zero and masks a shift count to its low six bits, so
setBit(-1) landed on longs[0] with 1L << 63 and flipped a live bit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
500 tests across the sheet models, tables, cell renderers and editors, the
script editor, the framework utilities and the legacy widgets. Core is not
involved except as data types: every fixture is a local test double, so a
failure here means the frontend is wrong, not that Core changed underneath it.
Assertions are derived from stated properties - algebraic laws, bijections,
symmetries, published format specs - rather than from recording what the code
currently does. Each property carries a comment giving the reasoning, so a
future reader can tell a real expectation from a rubber-stamped one. Where a
property and the code disagree, the test is left red rather than adjusted to
match: a suite that encodes existing bugs as expected behaviour is worse than
no suite.
The central properties, each generalising a bug this codebase has actually had:
- reads are pure. observing a model must not change it. this is the
learnsets bug (painting a sheet grew the file), stated as a law and
applied to every model. purity is checked with a reflective deep
fingerprint rather than equals(), because LearnsetData inherits
ArrayList.equals and would have defeated the check.
- writes are local. setting one cell moves exactly that cell.
- renderer and editor agree. what a cell paints and what opening it shows
must be the same value, or closing the editor silently rewrites the cell.
- text-source consumption is a bijection. the positional queue of name
lists must be walked identically by every consumer, or a column is handed
another column's names.
- parsers are total. malformed input fails diagnosably; never a raw NPE,
AIOOBE or StackOverflowError, and never on a paint path.
- advertised ranges are honest. a range the editor offers must be one the
storage can actually hold and return.
- tri-state tree consistency, re-verified over the whole tree after every
single mutation rather than only at the end.
Every property was mutation-checked: the defect it targets was reintroduced,
the test confirmed red and named the right cell, and the source restored.
Currently red, each documenting a defect left unfixed for review rather than
worked around:
- advertised cell ranges the storage cannot honour, and out-of-range writes
truncated silently at save (move 512 becomes move 0, level 200 becomes
level 72) via a paste path that bypasses the editor entirely
- XmlReader tests for a close tag spelled with a backslash, so it cannot
read well-formed XML at all, fails undiagnosably on most malformed input,
and accepts truncated documents as complete
- CsvReader cannot read a quoted field containing a line break, which the
export path writes - so an exported sheet is not re-importable
- exportEditable bounds its column loop by the width of the first row, so
a zero-row sheet throws
- getCornerTableHeader passes negative indices to getColumnName; it works
only because FormatModel adds the same offset back
- copy and paste feed the "nothing selected" sentinel straight into get(-1)
- ScriptDocument visitors build zero-width ranges from ANTLR error
recovery; the timer no longer lets that reach the EDT, but the visitors
should skip degenerate contexts (11 sites, left for review)
- ArrayProcessor loses trailing empty fields and NPEs on an empty record
- SheetExceptionFactory drops a parameter and duplicates a stack frame
- BitStream(0) cannot grow
- JarClassLoader.getMainClassName leaks a file descriptor
- JCheckboxTree.checkSubTree leaves the model inconsistent, NPEs on a null
model, and does not track structural changes
- CircleButton.getPreferredSize NPEs before display and ignores an
explicitly set size
Two components could not be covered and are reported rather than worked
around: DefaultSheetPanel and DefaultDataEditorPanel need a PokeditorManager,
whose constructor requires a real ROM; and CellTypes dispatch needs a
model-free seam. One test is disabled because the code under it hard-codes a
JOptionPane call that itself throws headlessly - a testability defect in the
production code, not a gap in the suite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
A column's declared range was documentation, not a rule. getCellValueRange
reached exactly one consumer - NumberOnlyCellEditor - which is only used for
INTEGER columns typed into by hand. A paste goes straight to setValueAt, and
combo box columns had no editor-side check at all, so a value the storage
could not hold simply landed in the data and was narrowed when the file was
written. Core now refuses to narrow silently, so without this the same paste
would fail at save time instead, which is worse: the user would learn about it
long after the edit that caused it.
The check moves to prepareObjectForWriting, which every write funnels through.
It takes the range rather than a column index deliberately: the enum handed to
setValueFor already carries its own range, so nothing has to be threaded and
there is no index to get wrong - LearnsetsColumn.repetition in particular is
mutable state on an enum constant, stale on the frozen-column path, and would
have been the natural thing to pass.
Paste is now checked as a whole before any of it is written. The write loop
cannot roll back and there is no undo, so rejecting a value part way through
would leave the sheet holding some of the paste and not the rest, with nothing
to say which - trading silent corruption for a half-applied edit. The dry run
costs nothing because the conversion is pure. The report names the offending
cells by column name rather than by view index, which matched no header the
user could see.
Also in the same funnel:
- Boolean.parseBoolean answers false for anything it does not recognise, so
pasting a spreadsheet column of 1s and 0s silently cleared every checkbox
it touched. 1/0, yes/no and true/false are understood; anything else is
refused rather than read as false.
- A name pasted into a numeric column produced parseInt's own message, which
does not tell the user what the column wants. The sheet exports rendered
text, so an exported column is full of names and this is the ordinary case,
not an edge one.
PersonalColumns TYPE_1/TYPE_2 declared 0..255 while Core refuses anything above
18. The declaration now mirrors Core's guess; both are still guesses, and the
comment says so.
The remaining live-code failures are fixed too:
- exportEditable bounded its column loop by the width of row 0, so a sheet
with no rows threw instead of exporting nothing.
- getCornerTableHeader passed getColumnName an index in [-n, -1] for every
column. It produced the right strings only because the frozen models add
the same offset straight back on - two wrongs cancelling. The frozen
wrappers now override getColumnName so their own naming is correct, and
the corner simply asks for the column it means.
- Copy and paste fed the "nothing selected" sentinel into get(-1), raising
IndexOutOfBoundsException from a toolbar button that is live from the
moment the editor opens. Paste then tried to raise a dialog about it from
inside the catch, which throws again headlessly.
- A short text-source queue threw NoSuchElementException with a null message,
naming neither the sheet nor the column that went unserved.
- ScriptDocument's visitors built zero-width ranges from ANTLR error
recovery. All eleven sites now go through one guard.
Fixes a regression from the previous commit: clearing the element ranges on
every edit included edits that change nothing, so inserting an empty string
discarded the tooltips and ctrl-click targets for a quarter of a second in
response to nothing happening. The tests caught it.
The cell-type dispatch moves out of DefaultTable.loadCellRenderers into
TableCellComponents.forType. It was only testable by reading the method's
source text and looking for each constant's name - a test that could not tell
a real dispatch from a mention in a comment and depended on the working
directory. It is now called directly. Custom-column sharing stays in the table,
where it belongs, and is asserted by identity rather than inferred from how
many text sources were consumed.
Tests: 520 total. 503 run in the build that must stay green, and it is green.
The other 17 assert properties that framework/ and gui_old/ classes do not
hold; none of those classes has a caller anywhere in src/main, so the failures
are specifications for whoever revives or deletes them rather than defects
anyone can hit. They are tagged and split into their own CI job whose expected
count is asserted, so that a fix and a regression are both build failures
rather than a number nobody reads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Validating the whole rectangle before writing any of it was right, but it
refused a value no column can hold rather than recognising a cell that holds
no value - and every spreadsheet produces those.
Excel, LibreOffice and Sheets all terminate a copied range with a newline.
Splitting on newlines with a negative limit keeps the empty field after it, so
a 2x2 copy arrives as three rows, the last holding one empty cell. The dry run
handed that empty string to Integer.parseInt, which threw, and the paste was
refused in full. Copying a block out of a spreadsheet and pasting it in - the
commonest gesture the sheet supports - wrote nothing at all on every numeric
and combo box column of every sheet.
Two things were wrong and both are fixed:
- the trailing newline is a terminator, not a row. Counting it also inflated
the tiling factor, so a paste into a larger selection repeated the wrong
number of times.
- an empty cell means "nothing here", not "write nothing here". It is now
skipped for numeric, checkbox and combo box columns, by both the dry run
and the write loop so the two agree cell for cell. Text columns still take
it as a value, because clearing a name is a real edit.
Neither was caught because TestSheet reported CellTypes.STRING for every
column, and prepareObjectForWriting does nothing at all for text - so nine
paste tests exercised the geometry and none of the conversion. The fixture can
now declare real types, its setValueAt converts the way the real sheets do,
and NumericPasteTest drives a paste through columns that actually validate.
Reintroducing either half of the fix turns it red.
Two testability defects made this possible and are fixed rather than worked
around, since both are the reason the gap existed:
- PasteAction read Toolkit.getDefaultToolkit().getSystemClipboard() inline,
which needs a display, so the paste path could only be reached by
reflectively replacing the AWT toolkit. It now goes through a method a test
can override, and returns null instead of throwing where there is no
display.
- the rejection path called JOptionPane unconditionally, which throws
HeadlessException from inside the error path and replaces the problem being
reported with a different one.
Also:
- the type range is derived from PersonalData.NUMBER_OF_TYPES rather than
restating 18. Gen 4 has 18 types numbered 0..17, and typeColors has 18
entries, so 18 itself is one past the end - the previous comment argued for
a bound its own reasoning ruled out.
- CellTypesTest.integerEditorHonoursTheDeclaredRange asserted isNotNull(),
which would pass against an editor that ignored the range entirely - the
exact fallback the previous commit added. It now drives values either side
of both bounds. The bound is enforced when the value is read, not by the
document filter, which accepts "128" quite happily.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
DataManager.getData takes a ROM and then ignores it: the cache is keyed by data class alone and never cleared. Opening a second ROM in one session returned the first ROM's parsed data for every format, and because the cache was populated nothing re-parsed - so saving wrote the first ROM's personal table, TM list and everything else into the second. The ROM parameter made it look as though this had been considered. The caches now record which ROM they hold and discard themselves when handed a different one. Compared by identity rather than equality: two ROMs loaded from the same file are separate objects with separate edits, and treating them as interchangeable is the same bug in a quieter form. Every entry point that takes a ROM goes through the check, so no route can skip it. An earlier commit claimed this was fixed by making PersonalParser's tables instance fields instead of static. It was not, and could not have been - the parser is bound as a Guice singleton, so there is one instance for the lifetime of the process either way. The static fields were worth removing on their own merits; they were not this bug. Also fixes resetData, which called dataMap.remove(newList) - passing a List to a map keyed by Class, so it removed nothing at all. The type range now derives from the number of type colours the sheet actually has, rather than restating a count that Core no longer enforces. It is a statement about what this table can display, which is where a limit like that belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Two regressions from the previous commits, and the false claims that hid them. TmCompatibilityTable asked DataManager for the move list with a null ROM. That was harmless while the ROM argument was ignored on a cache hit; once the caches became ROM-scoped it read as "a different ROM" and reset for it. Double-clicking a TM column header and picking a move therefore discarded every open sheet's parsed data, every unsaved-change flag and every code binary - and the binaries are populated exactly once, at startup, so nothing ever put them back. From that click onward every save reported a fatal error and reload silently did nothing. The session was unrecoverable, and the user's work was gone before the stack trace appeared. The move list is now passed in when the sheet is built, where a ROM is in hand. A null ROM is refused outright rather than treated as a no-op: a no-op would stop the crash while leaving the caches free to answer for the wrong ROM, which is the failure the scoping exists to prevent, and it would do it silently. useRom no longer clears the dirty flags. Whether unsaved work may be discarded is a decision to put to the user at the moment they switch ROMs, not one a cache coherency helper settles on their behalf - clearing them means the exit prompt goes quiet and the edits vanish without a word. Reassigning a TM's move was never marked dirty at all. The change lives on the parser rather than in any sheet's data, so the next save wrote it while the exit prompt insisted there was nothing unsaved. Both save confirmations now happen before anything is prepared. processDataList is not side effect free - PersonalParser writes the TM table straight into the shared arm9 buffer and PokemonSpriteParser does the same - so preparing first and asking afterwards left arm9 already modified when the user declined, and the next confirmed save of any sheet wrote that cancelled edit to disk. Reloading could not undo it either, because the reload re-reads the TM table out of the arm9 it had just mutated. The file list the dialog needs comes from the parser's own requirements, which is the same set the prepared output would have yielded, so nothing has to be serialised to ask the question. This also covers the git commit-message prompt, which was a second way to back out with arm9 dirty. Two comments claiming otherwise are corrected rather than deleted: useRom does not in fact cover every entry point that takes a ROM (commitData and saveCodeBinaries do not come through it), and the cross-ROM contamination it guards against cannot currently be reached at all - the three menu entries that would open a second ROM are unimplemented and closing the tool frame ends the process. It is kept as a guard against a future capability, and now says so. The field script editor is no longer built or shown. It is the one editor that compiles scripts back into the ROM, so a half-working version damages a project rather than merely disappointing; constructing it is also what pulls in VariableTracker, which is not published anywhere. Main no longer dies on a missing jokes file. It has never been committed, on any branch, and the resource stream was dereferenced unguarded - so a clean checkout could not start at all. Nothing cosmetic should be able to prevent launch. Tests for the caches, which had none. A refused call must not discard parsed data, code binaries or dirty flags; every entry point must refuse null the same way; and the file list must be obtainable without side effects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
…thers Personal, TM compatibility, Evolutions and Learnsets all read the same species name bank, across three different data classes. Adding or removing a row shifts that shared bank but fires table events only on the sheet being edited, so every other open sheet keeps displaying names that have moved underneath it - each one now labelling the entry above or below the one it belongs to. Nothing reports this. The sheet being edited looks correct, the others are only wrong if you go and look, and because the bank is marked dirty the next save of any sheet writes it to the ROM. One click on the Evolutions sheet renames every species from that row down. The other sheets are now told. The notification walks the open panels the same way resetAllIndexedCellRendererText already does, matches the bank by identity since the point is that these sheets hold the very same object, and fires a data change rather than a structure change - a structure change would discard the column widths and cell renderers the tables configure when they are built, and would not rebuild them. This corrects the view, which is what stops the silent wrong-row edit. It does not make the underlying operation sound: these tables are positional, so deleting a row renumbers every entry after it and anything referring to those entries by index now points somewhere else. Whether the row buttons belong on species-indexed sheets at all is a separate question, and the comment says so rather than implying the matter is settled. Also in this commit: codeBinarySetup now writes arm9 through the lock, like every other arm9 write. Not for mutual exclusion - nothing is concurrent at startup - but because lock() and unlock() are what maintain the buffer's cursors, extending the recorded size to whatever was written and returning the writer to the end. getData() returns the bytes between the cursors, so the hand-rolled save and restore this replaces was a weaker copy of the same protocol that could truncate arm9 on save. The fix removes code rather than adding it. Two more comments corrected. resetData claimed that prepareData no longer mutates the ROM; moving the confirmations ahead of preparation means a cancelled save leaves the ROM untouched, but preparation itself still writes to arm9, so a save that fails after confirmation can leave it partly written. And disabling the field script editor was described as removing the VariableTracker dependency, which it does not - the dependency is still declared and ScriptDocument still imports it, so a clean checkout still cannot resolve it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Reported: typing a move name into a sheet errors out, so the move has to be found in the list by hand. The combo boxes have type-to-search, and a search that does not land on an exact match leaves the selection empty rather than guessing. getCellEditorValue reported that empty selection as -1, which the cell range check then refused - so an edit that had simply not chosen anything came back as an error dialog. Before the range check existed the same -1 was written into the data and masked at save into whatever bit pattern it happened to make: a learnset move of -1 became move 511. So the check is right and the reporting was wrong. The editor now hands back the value the cell already held, which makes a selection of nothing a no-op - exactly what the numeric editor has always done with input it cannot use. Same treatment for the bitfield editor, which had the same shape and answered "no flags set" for a selection that had chosen nothing. Three tests: an edit that selects nothing keeps the old value, a real selection is still reported, and a value the column has no name for opens and survives being closed. That last one matters for hacked ROMs, where an index past the name list is legitimate data the editor must not quietly replace. Also adds TECH_DEBT.md, recording what this bug-hunting pass found and did not fix. Everything in it was reproduced before being written down, and the entries that are judgement calls rather than defects say so. The two worth knowing about: row add and delete are unsound on species-indexed sheets, because the tables are positional and deleting a row renumbers every entry after it; and in Nds4j, saving a scanned sprite destroys it unless its top-left pixel is index 0, because the decoder takes its key from the first word of the ciphertext. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Typing into a combo box in a sheet threw IllegalAccessError on Windows: class com.jidesoft.plaf.LookAndFeelFactory cannot access class com.sun.java.swing.plaf.windows.WindowsLookAndFeel (in module java.desktop) jide-oss picks its style with "lnf instanceof WindowsLookAndFeel", on the branch taken for every look and feel it does not recognise - FlatLaf, which is the one we set, among them. Every JIDE component reaches it, because JidePopup.updateUI() calls installJideExtension() and updateUI() runs from the JComponent constructor; we arrive there because EditorComboBox installs a ComboBoxSearchable whose search popup is a JidePopup. An instanceof resolves its class before it can answer false, so the platform decides how it breaks. On Windows java.desktop holds that package and does not export it, so resolution fails outright. Elsewhere no JDK ships the package at all, and the system-scoped WinLaF.jar at the repository root is what has been quietly satisfying it. Neither remedy covers the other case. Parent-first delegation finds java.desktop's copy first, so WinLaF.jar is shadowed on Windows and could never have helped there; Add-Exports naming a package a module does not have is ignored silently, so carrying it everywhere costs nothing. Both were reproduced and both verified, the Windows condition by patching the class into java.desktop so it was present but encapsulated, exactly as it is there. The runnable jar had no build at all - it was assembled by hand outside the repository, which is why the fix had nowhere to live. A dist profile now builds it, writes the manifest entry, and unpacks WinLaF.jar, which the shade plugin otherwise drops because system scope is neither compile nor runtime. The default build and CI are untouched. JideLookAndFeelResolutionTest guards the classpath half: nothing names that class, so WinLaF.jar reads as dead weight, and removing it turns the test red rather than surfacing as a crash when a user types a move name. The manifest half cannot be asserted from inside a JVM that has not built the jar yet, so it is documented instead. Also recorded in TECH_DEBT.md: JIDE is used for exactly one class, and replacing it would retire the system-scoped jar and this whole class of failure. Its os.version table also stops at 6.2, so on Windows 10 and 11 its own isWindowsVistaAbove() is false and the crash arrives via XPUtils. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Nothing said how. The README pointed at the Releases page, and the dist profile's own comment explains why the manifest entry exists without ever saying which command writes it. The build order for the three sibling libraries lived only in the CI workflow, so anyone building by hand had to read build.yml to discover that Nds4j has to be installed before the other two, and that a change spanning several repositories is developed on branches of the same name in each. Records the part that is easiest to get wrong: hand-assembling the jar, which is how it was produced until now, silently drops both halves of the JIDE look-and-feel workaround. The result starts cleanly and then dies the first time someone types into a combo box in a sheet - IllegalAccessError on Windows, NoClassDefFoundError elsewhere, and neither remedy covers the other case. Also states plainly that a clean checkout does not build yet, because VariableTracker is published nowhere the build can reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
The jar handed to testers was assembled by hand outside the repository, which
is how it came to ship without either half of the jide-oss look-and-feel
workaround. CI now builds it with the dist profile on every push and uploads
it as a run artifact, so a tester can take it from the Actions run.
Building it is not the same as checking it. Shade succeeds whether or not the
manifest carries Add-Exports and whether or not WinLaF's classes made it in,
so a build-only step would go green on precisely the artifact that crashes on
the first keystroke in a sheet. Both halves are executed instead:
- on the runner's own terms, which covers WinLaF being dropped - nothing in
src/main names that class, so it reads as dead weight, and losing it is a
NoClassDefFoundError for every Linux and macOS user;
- under the Windows condition, reproduced by patching the class into
java.desktop so it is present but encapsulated as it is there, and by
giving jide-oss an os.version it recognises, since its table stops at 6.2
and its own isWindowsVistaAbove() is false on Windows 10 and 11.
The exports applied are the ones the jar itself declares, so trimming the
manifest fails the step. A third run then withholds them and requires the
probe to fail: without that, a simulation that stopped reproducing the
condition would leave a step that passes forever and proves nothing. Both
failure modes were confirmed by breaking a built jar each way and watching
the step go red.
Reading the manifest needs the helper rather than grep. The value spans three
lines and the wrap currently falls inside sun.awt.shell; which tokens split
moves with the value, so a present package can read as absent and a split one
becomes a garbage --add-exports flag rather than an error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
ComboBoxCellEditor keeps the value a cell arrived with so that an edit selecting nothing leaves it alone - type-to-search empties the selection whenever the typed text matches no entry exactly, which is what made typing a move name raise an error instead of editing. BitfieldComboBoxEditor had the same guard and it did not work. The subclass overrode getTableCellEditorComponent without calling super, so the value was never recorded, and getLastValue() returned null: opening a flag cell and committing without picking handed back null rather than the flag. Not -1, and not the old value - the cell losing its contents. Mistyping a flag name was enough to reach it, as was merely opening a cell holding a bit the column has no name for, since that deliberately shows no selection. Recording the value is now final and subclasses override selectValue instead, so the mapping from value to entry can be changed while the bookkeeping cannot be skipped. This was the parent's own bug reappearing one level down, which is a sign the two were separable when they should not have been. The three tests covering this on the parent had no counterpart here, which is why the subclass shipped broken. Three are added, and reintroducing the defect turns two of them red; the third is the control that a genuine selection is still reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Auditing the other two editors for the defect fixed in the combo boxes found it in the numeric one, reachable and live. A Learnsets row is only as long as that species' learnset, and the read path deliberately returns null past its last entry rather than growing the list - scrolling must not invent moves. So most of that sheet is blank. Opening a blank cell and clicking away put a modal error in front of the user, saying "" is not a valid value, for an edit that changed nothing. A stray double click was enough. Committing it would have been worse than the dialog: setValueFor pads every entry up to the one being set, so writing to a blank cell injects junk moves, which is exactly what the read path is careful not to do. The edit is now cancelled rather than rejected - no dialog, and no write. Emptying a cell that held a value is still refused, because that is a real attempt to store nothing in a numeric column. The fallback that hands the cell back unchanged also stored String.valueOf(value), which turns a blank cell into the four letters "null" - so the value offered as the cell's new contents was text the write path then refused as not a number. It keeps the value itself now. Adds the join nobody was testing. The editors were covered alone and the write path was covered alone, and both stayed green while the sheet was broken, because the bug was in the handoff: a cleared selection reported as -1, and -1 is outside every column's range. EditorToWritePathTest runs the editor the sheet actually installs for a column and pushes what it produces into that column's write path. Reintroducing the original defect fails it with the error the user saw - "-1 is outside the range 0 to 511". CheckBoxEditor has the same shape and is not reachable; TECH_DEBT.md records why, and that what keeps it safe is a frozen column's isCellEditable rather than anything in the type declarations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Sweeping for open items not already written down turned up four, plus one that was written down wrongly. The sequencing section said Nds4j had to be published to Maven Central before the other three could merge. That is not true and it would have held up the merge for no reason: nothing in CI resolves these from Central. Each build clones the sibling at a branch of the same name, or main, and builds from source - so Nds4j merging to main is what unblocks the others, and publication matters to users and to the README coordinate, not to the build. Newly recorded: PokEditor's build fails at dependency resolution and skips every step after, so its pull request cannot go green while VariableTracker is unpublished, whatever else is fixed. The signing subkey has expired and is on only one of the two keyservers. Four repository secrets are still absent. maven-publish.yml publishes to OSSRH on JDK 8, which is decommissioned and returns 404, and sits alongside the release.yml that replaced it. And the one open decision: CodeBinary.compressed is private, unread, and reachable only by reflection from its own tests. Expose it or delete it, but not after 1.0.0 is permanent on Central. It is the only unresolved review thread across the four pull requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
isCompressed() is public and the OSSRH workflow is gone, so both stop being open questions. Records what the getter cannot say for itself - the flag is about the constructor argument, not the current contents - and that wasCompressed() would have been the better name, since that is the half no longer changeable. maven-verify.yml is noted rather than deleted: same vintage, but it fires on no branch, so it is neither running nor blocking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
It shipped as isCompressed() and was renamed before merge. The name mattered twice: it describes the constructor argument rather than the object, and Overlay already had an isCompressed() reading the overlay table bit, so the base-class method was silently overridden by a subclass answering a different question. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
maven-verify.yml looked like coverage for the scanned NCGR path and was not: it deleted the ROM it downloaded by checking out after fetching it, and ran on a JDK too old to compile the tests. Worth stating next to the coverage gap, because the workflow existing is why the gap looked smaller than it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
The section listed three defects and no note about coverage, which made it look like the thinnest of the four. It is the opposite: 3,834 lines of main against 588 of test, reaching two files. Tool.java has 1,299 lines and no tests, and all three recorded defects are in the untested files - FileUtils was the one part with tests and the one part that got fixed properly. Also states what the numbers hide in the other direction: the three atomic-write tests that skip locally are the ones with teeth, and they skip only because a container runs as root. CI is not root and runs all 25. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
A sheet's save writes several NARCs and arm9. It wrote them one at a time, so a failure part way through left the project holding a combination it was never in - a new personal.narc beside the TM table that was meant to change with it. ToolUI now stages a batch before replacing anything, so the failures that happen in practice leave the project entirely as it was. TECH_DEBT records the three items as fixed, and keeps what the fix does not cover: the run of renames at the end is not one operation, so a process killed between two of them still leaves a mixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
PokEditor declared JGit 6.7.0 and never referenced it. A direct dependency beats a transitive one, so the version ToolUI asks for was being overridden here - and upgrading ToolUI to fix the SSH-signing crash would have left the shipped application on the broken version with every test still green. Removed rather than bumped. Nothing here uses JGit, and re-pinning it is exactly how the two drifted apart. Verified in the built artifact rather than the dependency tree alone: the dist jar carries JGit 7 classes, and its manifest is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
jsvg belongs to flatlaf-extras, which is what draws the SVG icons; declaring it here pinned it a version ahead of what flatlaf asks for, with nothing importing it and so nothing to fail if the pin were wrong. jackson-dataformat-xml and junit:junit are unused outright - this module is entirely on JUnit 5. jackson-databind is declared instead, because DataManager does import it and was reaching it through the xml artifact by accident. flatlaf-intellij-themes and Guice stay: unlike ToolUI and Core, this module genuinely uses both. Verified on a clean build and in the shipped jar rather than the dependency tree - jsvg, the themes, Jackson and JGit are all present, and an SVG icon still renders from the jar itself, which is the only way to see a missing renderer. flatlaf reports nothing when it is absent; the icons simply come out blank. TECH_DEBT records the rule and the two traps: dependency:analyze cannot see runtime loading, and a dependency change reports green on stale classes unless the build is cleaned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
The uncaught handler printed the stack trace and told the user to see the command line. This is launched by double-clicking a jar, which on Windows runs through javaw and has no console - so the trace went to a stream with no reader and the dialog directed the user to something that does not exist. Both times a user reported a crash on this branch, the diagnosis had to be reconstructed from source because nothing had been kept. ToolLog.begin now runs first, before the handler and before anything prints, and the dialog names the log file so a report can carry the stack trace instead of one line of message. When no file could be opened it says that plainly rather than naming a path that is not there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
VariableTracker existed only as a local ~/.m2 1.0-SNAPSHOT and one un-remoted clone, so PokEditor could not build from a clean checkout and CI failed at "Verify dependencies resolve". Vendor the whole io.github.turtleisaac.variabletracker package (models, Swing panels, hex cell helpers, .jfd forms, and variable_tracker/*.properties) under src/main and drop the Maven dependency. Its only third-party needs - Jackson, FlatLaf, MigLayout - were already declared here. Add a ScriptVariable(String, int) constructor that neither the committed source nor the published artifact had, so ScriptDocumentHighlightingTest compiles. Mark the corresponding TECH_DEBT.md blockers resolved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Merging and releasing" section described a world that no longer exists. It said Nds4j merges first and that main is still 0.1.0; that the other three clone a sibling at a matching branch name and build it from source; that publication to Central is not a merge prerequisite; that four repository secrets must be created and the signing subkey extended before release.yml can run; and that PokEditor's build is red on the unpublished VariableTracker. Every one of those is now either done or wrong, which made the file's most operational section the least trustworthy part of it. Replaced with what is actually true, each point checked rather than recalled: - What is published. Nds4j 1.0.0 resolves from Central with jar, pom, sources, javadoc and signature all present; ToolUI and PokEditor-Core both 404. The signature is the evidence that the secrets exist and the key works, since release.yml has run once, on the v1.0.0 tag, and succeeded. - How to build the chain locally, which is now the only way to exercise a change spanning two repositories, and the better test anyway: it runs the exact combination about to ship rather than whichever branches happened to share a name. - That ToolUI's and Core's CI no longer build Nds4j from source, what that costs, and that PokEditor's CI still builds the two unpublished siblings because nothing else can resolve them. - That a version bump now propagates only by hand, and that Nds4j main declares 1.0.0 while sitting 8 commits past the tag of that name, having gained Screen and CellAnimation and a rewritten CellBank. So installing Nds4j main locally overwrites the published 1.0.0 in the local repository with different code -- worth knowing before following the local build steps. - That neither ToolUI nor Core has any publishing path: no release workflow, no publishing, gpg or javadoc plugin, and none of the six pom elements Central requires. Left as an open decision, with the case for each stated, rather than as a defect. Writing the local build steps turned up something the file should have said all along: a plain "mvn verify" on PokEditor goes red. 545 tests run and 17 fail, and all 17 are the @tag("dead-code") specifications that are meant to fail. CI passes -DexcludedGroups=dead-code, which is why CI is green; anyone following build steps without it would conclude the build was broken. The steps now carry the flag and say why. Checked both ways: 545 run with 17 red, 528 run and green with the exclusion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
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
2 participants
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.
Part of a four-repo audit of the PokEditor toolchain. Merge last — depends on turtleisaac/Nds4j#2, turtleisaac/Nds4j-ToolUI#1 and turtleisaac/PokEditor-Core#8, and includes the migration for Core's breaking API changes.
Corruption with no user edit
Merely rendering the Learnsets sheet injected junk moves into every Pokémon.
getValueFor— a read path called fromgetValueAtduring painting — grew the underlyingLearnsetData, appending entries until the list was long enough for whatever column was being painted. Padding entries serialize as0x0000, which is not the0xFFFFterminator, so Caterpie learned move #0 at level 0 eighteen times. Reopening read them back as real entries, so it compounded on every cycle.Opening the Priority editor destroyed negative move priority. The document filter stripped every non-digit with
replaceAll("\\D++", "")— including the minus sign — andsetTextgoes through the filter, so seeding the editor mangled the value before the user typed anything. Double-click Trick Room's priority cell (-7), click away, and it committed+7. One click, no typing. Same for Quick Attack, Roar, Whirlwind, Vital Throw, Focus Punch.The number editor validated nothing. The range check was
val >= 0 || val <= 255— always true — and it parsedlastValue, the value the cell held before editing. Typing300into Hyper Beam's Power wrote 44 to the ROM while the sheet kept displaying 300.Editing any species name rewired the Evolutions method dropdowns.
resetIndexedCellRendererTextconsumed the column text sources differently fromloadCellRenderers(which takes three entries for the firstCUSTOMcolumn), so every combo column after it got the wrongString[]. Picking "Charizard" from what looked like a method list wrotesetMethod(6)— "trade holding item". The positional queue is replaced by a column-keyed map so the two cannot drift again.Data loss
stopCellEditing()appeared nowhere in the project andterminateEditOnFocusLostwas never set. Type a value, click Save without pressing Enter, and the ROM got the old one while the editor still showed the new one on screen.FieldScriptModel.setValueForwas an empty method whose body was commented-out sprite-editor boilerplate.CommandMacro.writeand aborting the entire ROM write — for all field scripts, not just the open one. Substitution now happens only in the display path.Crashes
Importing an indexed PNG with fewer than 16 colours (the normal GIMP/Aseprite "optimal palette" result) threw partway, leaving the palette half-updated and the sprite not imported, with no dialog. Importing into an empty sprite slot produced a placeholder whose scan mode was unset and saved as an all-black sprite — the underlying cause is fixed in Nds4j#2. Importing a party icon overwrote the battle sprite palette.
Ctrl+C/V/Ain the script editor threw whenever the pointer wasn't over the pane, so copy/paste silently did nothing. Undefined labels and stale jump-list entries were used as unchecked offsets. The battle mockup opened a modal dialog from insidepaint, which reopened itself forever once dismissed. Delete Row threw when the row was selected via the frozen column. Paste computed its copy count withMath.round, so the commonest gesture (one cell selected, three rows pasted) pasted nothing and reported nothing, while a larger selection overwrote a row below it.Honesty
hasUnsavedChanges()returned a hardcodedfalse, so closing after an hour of edits exited without a prompt.ConsoleWindowwas never instantiated and its stream redirect was commented out, so every validation failure went to a stream with no window attached — failures are now reported next to the cell, with an uncaught-exception handler installed. Export reported "Success!" when the chooser was cancelled.Dead controls that looked functional: the Find dialog had no listeners and no search logic (implemented),
deleteCurrentEntry()was an empty override (implemented), and the two shiny-palette buttons had no listeners (removed rather than left lying — "Set to Shiny" has genuinely ambiguous semantics and shipping a wrong palette write is worse than no button).Syntax highlighting re-lexed the whole document with ANTLR twice per keystroke on the EDT, including for arrow keys; it is now debounced.
Framework utilities:
BitVector's mask set two bits instead of one,CsvReaderdropped trailing empty fields and used the platform charset,ArrayModifieraborted on the first short row,Directory.delete()always claimed success, andTrainerPersonalityCalculatorproduced a wrong TID from every seed.Not reported
I checked view/model index handling first, since it's the classic
JTablebug. It is not reachable: noRowSorteris installed anywhere and both tables disable column reordering, so view index == model index. A suspectedMath.logprecision bug in the bitfield editors was compiled and disproved. Recording these so they don't get re-litigated.pom.xmlrequiresio.github.turtleisaac:VariableTracker:1.0-SNAPSHOT, which is unpublished with no<repositories>entry. No one can build this repo from a clean checkout — I checked all 30 repos on the account and it isn't there.Everything here is compile-verified against a local stub matching the exact API surface the code uses (
ScriptVariable,VariableTracker,FlagTracker). I deliberately did not commit that stub — vendoring it would silently replace a real feature with no-ops. You'll need to publish VariableTracker, add a<repositories>entry, or vendor it yourself. Until then this PR cannot be CI-verified.Verification
mvn compile→ BUILD SUCCESS against the three companion branches plus the local stub. No test suite exists in this module; the logic under test lives in Core and Nds4j, which are covered by their own PRs.🤖 Generated with Claude Code
https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob
Generated by Claude Code