Random-name dialog: rewrite from Swing to JavaFX#7566
Open
Vest wants to merge 25 commits into
Open
Conversation
Never packaged into any *plugins.jar, no callers — unreachable since GMGen was retired. The live name generator path (RandomNameDialog → NameGenPanel) is unaffected.
The 'doomsdaybook' name is a 2003 artifact of the third-party product PCGen adopted the engine from. Rename clarifies the package's purpose.
Mirrors the engine package rename. NameGenPanel, NameButton, and XMLFilter move; RandomNameDialog updates its single import.
Pull XML loading, catalog/category/gender selection, and rule invocation out of pcgen.gui2.namegen.NameGenPanel into a new UI-toolkit-independent facade in pcgen.core.namegen so the engine can be reused from JavaFX, a CLI, or tests without a Swing runtime. NameGenerator exposes getCategories / getTitlesFor / getGendersFor / getCatalog / generate / generateWithRule / getRulesFor. NameGenDataLoader handles the directory scan and per-file XML parse (still JDOM2 — translation to javax.xml.parsers comes next). NameGenData is the immutable snapshot; GeneratedName carries the assembled name plus meaning and pronunciation. NameGenPanel now constructs NameGenerator and delegates; ~370 lines of loading/generation logic move out of the UI class. User-visible behavior unchanged.
Pin NameGenerator's facade behaviour and the loader's handling of the bundled plugins/Random Names dataset before the JDOM2 → javax.xml.parsers swap. Loader tests cover the happy path, the internal Sex:/All buckets, non-directory inputs, broken XML, and empty directories. Generator tests cover category/title/gender filtering, canonical gender ordering, catalog resolution, 100 repeated generations producing non-blank names (schema-based — Dice isn't seedable), and that generateWithRule honours the forced rule.
Mechanical translation: SAXBuilder → DocumentBuilder, Element methods to their org.w3c.dom counterparts, and a small childElements helper to filter NodeList down to elements (since the DOM API exposes whitespace text nodes between elements that JDOM2 hid). Integer parsing moves from getIntValue (checked DataConversionException) to Integer.parseInt; the loader catches NumberFormatException alongside SAXException and rethrows as IOException so callers see the same failure shape as before. Mixed-content <VALUE>foo<SUBVALUE>...</SUBVALUE></VALUE> elements in gaelic.xml exposed a real divergence: getTextContent concatenates descendant text, while JDOM2's getText returned only direct child text. Added a directText helper that walks direct TEXT_NODE / CDATA_SECTION_NODE children, plus a regression test pinning the "Donn / brown, brown-haired" entry. JDOM2 is still on the classpath; the dependency drop is the next commit.
Last call into JDOM2 went away with the previous commit's loader rewrite. Drop the runtime dependency and module require, plus every JDOM mention left in source — the lingering "we used to use JDOM2" comments would rot otherwise. About-dialog attribution strings (in_abt_lib_jdom across en/es/fr/it bundles) and a broken-since-forever Src.iml jar reference go too.
This IntelliJ module descriptor predates the Gradle build. It references lib/jdom.jar and lib/MRJ141Stubs.jar — neither exists in the repo — and has been functionally dead since the project moved to Gradle. .gitignore already excludes the .iml files that modern IntelliJ generates from the Gradle import.
The plugin generated IntelliJ project files via ./gradlew idea, a workflow modern IntelliJ supersedes by importing build.gradle directly. The previous developer's TODO already noted that the generated source-set wiring was broken in current IntelliJ, and .gitignore excludes the .iml files the Gradle import would produce — confirming nobody runs the idea task.
Introduces pcgen.gui3.namegen with an FXML-driven panel that drives the headless NameGenerator: two cascading combo boxes (Category > Title), three gender radio buttons with sticky-fallback semantics, and Generate / OK / Cancel actions. The dialog wrapper mirrors the existing Swing API (getChosenName / getGender) over JFXPanelFromResource so Swing callers can switch over in a follow-up commit. The old Swing panel still works; nothing is wired to the new dialog yet.
Main and SummaryInfoTab now invoke pcgen.gui3.namegen.RandomNameDialog through showAndBlock() instead of constructing the old Swing dialog. With both call sites converted, the original Swing implementation is deleted in full: NameGenPanel, NameButton, XMLFilter, and the gui2 RandomNameDialog wrapper. The empty pcgen.gui2.namegen package goes with them. Also drops a stale reference to NameGenPanel from a NameGenDataLoader javadoc, and removes the now-unused JFrame plumbing from RandomNameAction in SummaryInfoTab.
The previous wiring went through JFXPanelFromResource, which is a
JFXPanel subclass: its scene is owned by the embedded Swing surface,
so reusing it inside a fresh Stage on showAndBlock blew up with
SceneState NPEs from the embedded toolkit. The dialog isn't actually
embedded in Swing — it's standalone-modal — so it can load the FXML
straight into a Stage and avoid the JFXPanel layer altogether.
Also drops a stray Logging.errorPrint("passed wait") line in
JFXPanelFromResource.showAndBlock.
Extract the sticky-gender decision out of the FX controller into a pure GenderSelection helper, and exercise the four branches headlessly. Add a TestFX smoke test mirroring AboutDialogTest that loads the panel's FXML and verifies the expected controls render — guards against FXML breakage and missing fx:id wiring.
A collapsible TitledPane labelled "Adjust Name:" exposes a "Random" checkbox (default on) and a Structure ComboBox listing the rules available for the current (category, title, gender). With the checkbox unchecked and a structure selected, Generate routes through NameGenerator.generateWithRule so power users can pin a specific naming pattern instead of taking a weighted random one. The structure list refreshes whenever the catalog changes (gender selection toggle settles), and the combo is disabled while random mode is on.
# Conflicts: # build.gradle
childElements(parent, tag) used to call childElements(parent) and filter the resulting list, traversing the NodeList twice. Inline the filter so each call walks the children once. Also drop the pre-sized ArrayList in childElements(parent) — Stream.toList() returns a right-sized immutable list directly.
The DTD declares weight="1" as the default for VALUE and RULE, but the loader used a non-validating parser, so a missing attribute came through as "" and crashed the entire load with NumberFormatException. Read it through a parseWeight helper that treats blank as 1, matching the DTD's stated default. Also disable external general/parameter entities, XInclude, and entity-reference expansion on the DocumentBuilderFactory. External DTD loading is left on so generator.dtd still resolves through the EntityResolver. The data files are local and trusted, but XXE hardening is cheap and stops a malformed file from silently inlining filesystem contents. Tests cover both: a VALUE without a weight attribute now loads without throwing, and an XXE payload referencing a local file does not leak the file's contents into the parsed data.
Replace ArrayList-of-IDs subclasses (Rule, RuleSet) and the VariableHashMap registry with records + a sealed RulePart interface. References between rulesets, rules, and lists are linked at load time via a two-pass loader instead of being re-resolved by string lookup on every name generation. Also adds a NameGenData.unresolvedReferences() collection so dangling GETLIST/GETRULE refs surface as data instead of being silently swallowed at runtime, with tests covering both the synthetic case and the bundled dataset. Net: ~1,000 lines removed across 9 deleted files (variable/operation machinery for the never-used VARMOD/ROLL/IF/WHILE DTD nodes).
The bundled dataset contained 33 unresolved id refs that the legacy engine swallowed silently at generation time. The new loader surfaces them via NameGenData.unresolvedReferences(), which is asserted empty in NameGenDataLoaderTest. Categories of fix: - 25 GETLIST refs whose target is a RULESET → switched to GETRULE - 4 GETRULE refs whose target is a LIST → switched to GETLIST - tamrelilic-noun-a typo → tamrelic-noun-a (7 occurrences) - tamriel-dunmer-surname-Suffix case typo → ...-suffix - 6 mar-standard-rule refs split into mar-standard-male-rule / mar-standard-female-rule per call-site gender - russian-historical-male-patrynomic-rule_alt (never authored) → drop the _alt suffix to point at the existing rule
The structure combo only refreshed via the gender-toggle listener. Switching titles within the same gender bucket left the toggle's selected value unchanged, so onGenderChanged never fired and the combo kept the previous title's rules even though the underlying catalog had moved on.
Vest
commented
May 24, 2026
| } | ||
|
|
||
| // Support for creating IntelliJ IDEA files. | ||
| idea { |
Contributor
Author
There was a problem hiding this comment.
With better Gradle script we don't need Idea as a separate plugin anymore.
Contributor
Author
|
XMLs were corrected by the bot. UI, too. |
The datatest, inttest, and per-game inttest variants (pfinttest etc.) were silently running zero tests: include globs were rooted at `slowtest/` / `main/`, but Gradle compiles to `build/classes/java/slowtest/` so includes are relative to the source set output. Tasks were also missing `useJUnitPlatform()`, so JUnit 5 tests would not have been discovered even with corrected paths. Drop `forkEvery=1` on the per-game variants. Sharing one JVM cuts pfinttest wall-clock from ~50s to ~30s on a 4-class suite by avoiding 4 cold JVM starts and reloading plugins/game-modes/campaigns once instead of per class. The umbrella `inttest` and `slowtest` tasks keep `forkEvery=1` until cross-test isolation is verified.
The cobra-0.98.4 jar/pom under installers/lobobrowser was published via a 'local' maven repo declaration but no module depended on it. Remove the artifact, the stale .iml beside it, and the now-pointless repo entry.
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.
I have changed the window that generates a random name from Swing to JavaFX:
Drop the legacy namegen engine, fix data, fix UI bug
This branch retires the
VariableHashMap-based name generator (a port of the original Doomsday Book engine) and replaces it with a record-based model. Along the way it surfaces and fixes ~33 broken refs in the bundled name data that the old engine had been silently swallowing, and fixes a UI bug uncovered during testing.1. Replace VariableHashMap engine with record-based model (
0299fa3)Why: the old engine routed every parsed XML element through a
VariableHashMapkeyed by string IDs, withRule extends ArrayList<String>andRuleSet extends ArrayList<String>storing child references as ID strings re-resolved on every name generation. The indirection existed to support runtime variables (<VARMOD>,<ROLL>,<IF>,<WHILE>from the DTD) — features the DTD itself calls "unsupported" and that PCGen never used.What changed:
NameList,Rule,RuleSet,NameGenDataare now records.RulePartis a sealed interface withListRef,RuleSetRef, and aLiteralenum (SPACE / HYPHEN / CR).NameGenDataLoadernow uses a two-pass linker — pass 1 parses the DOM and harvests raw<LIST>and<RULESET>elements, pass 2 builds the immutable records and resolves cross-references via shared maps. Forward refs between rulesets work without ID re-resolution at runtime.NameGenData.unresolvedReferences()collects every danglingGETLIST/GETRULEso dataset bugs surface as data instead of being silently dropped at generation time.VariableHashMap,DataElement,DataElementComperator,Operation,variableException,DDList,SpaceRule,HyphenRule,CRRule.NameList,RulePart.NameGenDataLoaderTest,NameGeneratorTest); new tests cover the synthetic dangling-ref case and assert the bundled dataset is clean.2. Fix broken GETLIST/GETRULE references in random-name data (
cdbe72c)The new loader's clean-data test failed on first run, exposing 33 unresolved id refs in
plugins/Random Names/*.xmlthat the legacy engine had been swallowing. Categories of fix:<RULESET>, not a<LIST>→ switched toGETRULE.<LIST>, not a<RULESET>→ switched toGETLIST.tamrelilic-noun-a(×7) →tamrelic-noun-a;tamriel-dunmer-surname-Suffix→...-suffix.mar-standard-rulerefs — the unified id never existed; split intomar-standard-male-rule/mar-standard-female-rulebased on each call-site's gender ruleset.russian-historical-male-patrynomic-rule_altrefs — the_altvariant was never authored; dropped the suffix to point at the existing rule.Files touched:
arabic.xml,fantasy.xml,fantasy-tamriel.xml,fantasy_craft.xml,french.xml,gaelic.xml,gothic.xml,horror.xml,pictish.xml,russian.xml.