Random-name dialog: rewrite from Swing to JavaFX#7566
Merged
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.
Pre-scan files via StAX at startup for category/ruleset/list metadata, then full-DOM parse only the file behind the user's selection (and anything it transitively references). Cuts cold dialog-open from ~150ms to ~40ms on the bundled dataset.
Add a per-ruleset DynamicTest that calls generate() on every final-usage ruleset in plugins/Random Names. Catches damage that parse + cross-ref checks miss: empty lists, all-zero weights, content-free rules. Adds 144 tests / ~0.6s to :test.
Extract repeated XML attribute/tag literals as constants, switch the title-resolver functional parameter to UnaryOperator<String>, and use LinkedHashMap.newLinkedHashMap. Also guard against a null Rule from RuleSet.pick() and modernize getFirst() in the lazy test.
Split the 79-line state-machine into a small dispatcher and a private ScanState class with one method per element kind. Drops cognitive complexity, variable count, and nesting depth well below Sonar's thresholds. Behaviour is unchanged.
commons-lang3 is already on the classpath and used widely; no need for a one-line nullToEmpty utility inside NameGenIndex.
Both private methods carry behaviour a future reader can't infer from the name: the LIST-body skip via depth counter, and the cycle handling via late resolution through the shared RuleSetRef map.
Pass 2a (lists) and pass 3 (categories) are pure transformations producing one map each — natural fits for Collectors.toMap. Pass 1 stays a loop because parseOne throws checked, and pass 2b stays a loop because RuleSetRef reads the rulesets map mid-iteration.
Single-line `enum Kind { GETLIST, GETRULE }` tripped Checkstyle's
LeftCurlyCheck. Split onto separate lines.
Replace the GeneratorDtdResolver class with a static factory returning an EntityResolver lambda; swap prepareFileForLazy's local-rulesets loop for a Collectors.toMap.
Convert the pure transform-and-collect loops in NameGenIndex.unmodifiableDeep, NameGenLazyData.rulesetMetaFor / gendersForRuleset, and NameGenerator.getData's category map. Each is a single-input single-output pure transformation.
Contributor
Author
|
@karianna I thought, I reduce the code base much, but I wanted to have it properly implemented, it hasn't reduced much :( |
3 tasks
karianna
pushed a commit
that referenced
this pull request
Jun 4, 2026
`in_plugin_randomname_name` and `in_mn_plugin_randomname_name` were referenced only from `plugin/doomsdaybook/RandomNamePlugin.java`, the GMGen plugin wrapper that advertised the "Random Names" tab name and mnemonic. PR #7566 deleted that wrapper as part of the Random-name dialog Swing → JavaFX rewrite, and the live core dialog (`RandomNameDialog` → `RandomNamePanelController` + `RandomNamePanel.fxml`) doesn't expose a plugin-style tab label, so the keys have no replacement in core. Confirmed unused via `ScanForUnusedIl8nKeysTest` and a manual grep over `*.java`, `*.fxml`, `*.lst`. Removed from the four locale bundles that still carried them (en, es, fr, it; de/ja/pt already lacked them) and regenerated the `cleaned.properties` snapshot the scanner produces.
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.
I have changed the window that generates a random name from Swing to JavaFX:
Pulling the engine out of the Swing panel exposed enough rot in the underlying code (never-used variable-substitution machinery, an outgrown JDOM2 dependency, ~33 silently-broken refs in the bundled data) that fixing those became part of the same delivery.
1. JavaFX UI
Swing
NameGenPanelreplaced by an FXML-driven JavaFX panel + controller, hosted in a SwingJDialog. The engine was extracted into a UI-agnosticNameGeneratorfacade first (~370 lines moved out of the UI class) so the new panel could drive it cleanly.Stage(not viaJFXPanelFromResource) — the dialog is standalone-modal, not embedded, and the JFXPanel route blew up withSceneStateNPEs.Removed: dead
RandomNamePluginGMGen wrapper (no callers, never packaged).2. Replace
VariableHashMapengine with record-based model (0299fa3)The old engine routed every parsed XML element through a
VariableHashMapkeyed by string IDs, withRule extends ArrayList<String>storing child references re-resolved on every name generation. The indirection existed to support<VARMOD>/<ROLL>/<IF>/<WHILE>— features the DTD itself calls "unsupported" and that PCGen never used.NameList,Rule,RuleSet,NameGenDataare now records;RulePartis a sealed interface withListRef,RuleSetRef, and aLiteralenum.NameGenData.unresolvedReferences()collects dangling refs so dataset bugs surface as data instead of being silently dropped.3. Fix broken
GETLIST/GETRULEreferences in bundled data (cdbe72c)The new loader's clean-data assertion failed on first run, exposing 33 unresolved id refs the legacy engine had been swallowing:
GETLISTrefs targeting a<RULESET>→ switched toGETRULE.GETRULErefs targeting a<LIST>→ switched toGETLIST.tamrelilic-noun-a(×7) →tamrelic-noun-a;tamriel-dunmer-surname-Suffix→...-suffix.mar-standard-rulerefs split into-male-rule/-female-ruleper call-site gender.russian-historical-male-patrynomic-rule_altrefs — the_altvariant was never authored — pointed at the existing rule.4. Drop JDOM2
Loader rewritten on
javax.xml.parsers. One real divergence: mixed-content<VALUE>foo<SUBVALUE>...</SUBVALUE></VALUE>elements need adirectTexthelper becausegetTextContentconcatenates descendant text where JDOM2'sgetTextreturned only direct children. Regression test pins the "Donn / brown, brown-haired" entry. Dependency, module require, about-dialog attribution strings, and a long-brokenSrc.imljar reference all gone.Engine package renamed
pcgen.core.doomsdaybook→pcgen.core.namegenfirst — "Doomsday Book" was a 2003 artifact of the third-party engine PCGen originally adopted.5. Lazy XML loading
The full-DOM parse of the 4 MB
Random Namesdirectory was running just to populate the dialog's category list. Replaced with two tiers:<RULE>/<VALUE>bodies).GETLIST/GETRULE.NameGenerator's public API is unchanged. Cold dialog-open: ~150ms → ~40ms on the bundled dataset, measured via JMH (benchmark removed before merging once the numbers were in).6. Tests + parser hardening
@TestFactorycallsgenerate()on everyfinal-usage ruleset and asserts non-empty output. Catches empty lists, all-zero weights, content-free rules. +144 tests / ~0.6s to:test.datatest/inttest/ per-game variants were silently running zero tests — include globs were rooted atslowtest/instead ofbuild/classes/java/slowtest/, anduseJUnitPlatform()was missing. Per-game variants now share a JVM, cuttingpfinttestwall-clock ~50s → ~30s.weight="1"default: a non-validating parser was producing""and crashing the load withNumberFormatException. Helper treats blank as 1.childElementswalks theNodeListonce instead of twice.7. Build cleanup
Src.iml(predates Gradle, references nonexistent jars), the Gradleideaplugin (superseded by direct Gradle import), and an unusedlobobrowser/cobra-0.98.4artifact all dropped.