Fix #6517: language chooser modal hang and empty available-language list

[Vest]

Summary

Fixes both observable symptoms of #6517. Two independent commits, each fixing one of them, plus a SonarQube cleanup pass on the touched files.

1. LanguageChooserDialog: dispatch OK/Cancel to the EDT (commit 0940b62)

The dialog is a Swing JDialog (super(frame, true), modal) with an embedded JavaFX OKCloseButtonBar wrapped via GuiUtility.wrapParentAsJFXPanel(...). OKCloseButtonBar wires its OK/Cancel buttons with setOnAction(...), so the supplied handlers — this::doOK and this::doRollback — fire on the JavaFX Application Thread. Their bodies were calling chooser.commit() / rollback() (which mutate Swing-side models) and dispose() directly from that thread; on the affected platforms the modal event pump never advanced past those calls and the dialog locked up.

Wrap the bodies in SwingUtilities.invokeLater(...) so the Swing-side work runs on the EDT, where JDialog.dispose() and the Swing model mutations belong. doAdd / doRemove are intentionally not changed: they aren't reported as broken, and the existing DoubleClickActionListener path already runs on the EDT.

2. LanguageTableModel: flush deferred ComboBox commits before opening chooser (commit 0addaff)

The reporter and follow-up comments described a second symptom: opening Add Bonus Language right after picking a Race shows an empty available-list, and "doing something else" (e.g. changing Age) appears to fix it. Live debugging confirmed the cause:

The Race / Gender / AgeCat / Hand / Deity ComboBoxes on the Summary tab use DeferredCharacterComboBoxModel, which holds the selection back until focusLost. If the user picks Race=Dwarf and clicks "Add Bonus Language" before any other focus change, the Race ComboBox still has focus at the moment the chooser is constructed — theCharacter.getDisplay().getRace() is still <none selected>, so the chooser's LANGBONUS candidate set is empty.

When the cell editor's Add action fires, look up the focused component; if it's a JComboBox backed by DeferredCharacterComboBoxModel, call commitSelectedItem(getSelectedItem()) synchronously so e.g. setRace runs before the chooser facade is asked for its available list.

Subtlety: the chooser facade must be resolved before the flush. setRace mutates the languages list (auto-granted languages get added), which fires a table model event that cancels the cell editor — table.getEditingRow() then returns -1 and choosers.getElementAt(-1 - languageCount) would fail with IndexOutOfBoundsException.

3. SonarQube cleanup on the touched files (commit 5353c0a)

Drive-by cleanup of pre-existing findings on LanguageChooserDialog and LanguageTableModel:

• Mark non-Serializable fields transient (S1948 ×6). Swing's Serializable claim has been vestigial since ~2000 — these dialogs/models are never actually serialized — but transient is the cheapest way to silence the rule.
• Remove unused Collectors import (S1128); replace Collectors.toUnmodifiableList() with .toList().
• Pattern-match instanceof Language language && ... instead of instanceof Language && (Language) value (S6201 ×2).
• Method references Language.class::isInstance / Language.class::cast instead of o -> o instanceof Language / o -> (Language) o lambdas (S1612 ×2).
• Drop two stale commented-out lines (S125 ×2); fix a stray ; in a //$NON-NLS-1$ marker that S125 was misreading as commented-out code.
• Add a one-line // no-op: this view is read-only to an empty setData (S1186).
• Shorten the verbose Javadoc / inline comments added in commits 1–2 — the technical narrative belongs here, not in the code.

Test plan

• ./gradlew :test --rerun-tasks — full unit suite green (17053 tests, 0 failures, 0 errors) on the modal-hang fix; cherry-pick of the second commit is a clean apply touching only LanguageTableModel.
• ./gradlew compileJava clean after the SonarQube pass.
• Modal-hang manual repro (issue's primary scenario): SRD 3.5 → New → Int=12 → Race=Dwarf → tab to lose focus → Add Bonus Language → pick a language → OK. Without the fix the dialog locks up. With the fix it closes immediately and the language is added. Same for Cancel.
• Empty-list manual repro (the second scenario): SRD 3.5 → New → Int=12 first, then pick Race=Dwarf in the dropdown, then click Add Bonus Language without tabbing away. Without the second fix the available list is empty (race wasn't committed because the ComboBox still had focus). With the fix the available list shows the 6 expected languages (Orc, Goblin, Terran, Undercommon, Giant, Gnome for Dwarf).
• Verified both fixes live in the IntelliJ debugger.

Not covered by automated tests. Both bugs are Swing-modal interactions that only reproduce against a real JDialog.setVisible(true). The existing test config sets java.awt.headless=true, so a regression test would require a separate non-headless test task. Verification is the manual repros above.

Out of scope

• The "Langauge choice" title typo from the GM-Awards path is a separate data fix in data/pathfinder/alluria_publishing/remarkable_races/rr_abilitycategories.lst (the ABILITYCATEGORY:Kval Langauge / Muse Langauge lines plus matching BONUS:ABILITYPOOL references).

[karianna]

I'm a little suspicious of the use of transient, it's a valid keyword and JVM feature, but very rarely used and not that well understood. Let's see if Copilot can find another way.

[Copilot]

Pull request overview

Fixes issue #6517 in the Swing/JavaFX hybrid language chooser workflow by ensuring UI/model mutations occur on the correct thread and by committing deferred Summary-tab ComboBox selections before computing available bonus languages.

Changes:

• Dispatch LanguageChooserDialog OK/Cancel handlers from the JavaFX thread to the Swing EDT before calling commit()/rollback() and dispose().
• In LanguageTableModel, flush a pending DeferredCharacterComboBoxModel selection (e.g., Race) before opening the language chooser so the available-language list is populated correctly.
• Minor cleanup in the touched files (e.g., stream usage, removing stale commented code, Sonar-related transient marking).

Show a summary per file

File	Description
code/src/java/pcgen/gui2/tabs/summary/LanguageTableModel.java	Flushes deferred ComboBox commits before constructing the chooser; minor refactors/cleanup in renderer/model fields.
code/src/java/pcgen/gui2/dialog/LanguageChooserDialog.java	Ensures OK/Cancel operations run on the Swing EDT (fixing modal hang) and includes small cleanup/refactors.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 0

[karianna]

Thanks for chasing this one down — the two bug-fix commits are great:

• 0940b62 (EDT dispatch on OK/Cancel): the FX-Application-Thread → Swing-modal-pump diagnosis is exactly right, and SwingUtilities.invokeLater(...) is the right shape of fix.
• 0addaff (flush deferred ComboBox commits before opening the chooser): nicely explained, and resolving the chooser before the flush to dodge the IndexOutOfBoundsException from the cancelled cell editor is a nice touch.

One suggestion on the Sonar cleanup commit (5353c0a): I'd like to ask that the 6 transient modifier additions be dropped, while keeping the rest of that commit intact. My reasoning:

PCGen has an explicit, documented "we don't use serialization" stance

The project's own quality gate already turns off serialization-related rules with that exact rationale:

code/standards/ruleset.xml (PMD):

<!-- we don't use serialization -->
<exclude name="NonSerializableClass"/>
...
<!-- we don't use serialization -->
<exclude name="MissingSerialVersionUID"/>

code/standards/spotbugs_ignore.xml (SpotBugs):

<!-- not relevant to pcgen -->
<Bug pattern="...,SE_NO_SERIALVERSIONID"/>

SonarQube isn't part of the PCGen CI quality gate either — no sonar references in build.gradle, code/gradle/*.gradle, or any GitHub Actions workflow. And the PR description itself concedes the point: "Swing's Serializable claim has been vestigial since ~2000 — these dialogs/models are never actually serialized."

So S1948 here is being satisfied against a rule the project has, by its own published configs, consciously declined to enforce. Adding transient (a) puts source-level noise on every non-serializable field, (b) perpetuates the vestigial Serializable fiction one keyword at a time, and (c) sits awkwardly against the project's own "we don't use serialization" stance.

Alternatives I considered

Option	Verdict
A. Drop the 6 transient additions; keep the rest of 5353c0a	Recommended. Zero source noise; consistent with the existing PMD/SpotBugs exclusions; bug fix lands focused.
B. Class-level @SuppressWarnings("java:S1948") on the 2 classes	Acceptable fallback if SonarLint suppression is genuinely needed for a SonarCloud workflow — but that's a project-wide policy question, not something this bug-fix PR should decide unilaterally. One annotation per class instead of 6 per-field modifiers.
C. Explicit serialVersionUID + @Serial writeReplace() throwing NotSerializableException	Most semantically honest, but directly contradicts the project's "we don't write serialization plumbing" stance and adds boilerplate for no user-visible benefit.
D. Per-field transient (current PR)	Idiomatic Sonar-textbook fix, but the only piece of the PR that conflicts with the project's documented stance.

What's not being asked

The non-transient parts of 5353c0a are unambiguous improvements aligned with the project's actual Checkstyle/PMD/SpotBugs gate and should land as-is:

• removing the unused Collectors import
• .toList() instead of Collectors.toUnmodifiableList()
• pattern-matching instanceof Language language && …
• method references Language.class::isInstance / Language.class::cast
• deleting the two commented-out lines
• the stray ; fix in the //$NON-NLS-1$ marker
• // no-op: this view is read-only on the empty setData override

Happy to be overruled on the transient call if there's a SonarCloud workflow I'm not seeing — just wanted to flag the inconsistency with ruleset.xml / spotbugs_ignore.xml before this lands.

[Vest]

Thanks @karianna — fully agree, the inconsistency with the project's own ruleset.xml / spotbugs_ignore.xml is real and your option A is the right call.

Just force-pushed:

• Dropped all 6 transient modifier additions from LanguageChooserDialog (chooser, treeViewModel) and LanguageTableModel (languages, choosers, character, mouseListener).
• Updated the cleanup commit message — removed the S1948 bullet and added a short paragraph documenting why S1948 is intentionally unaddressed here (citing the NonSerializableClass / MissingSerialVersionUID exclusions in ruleset.xml, the SE_NO_SERIALVERSIONID exclusion in spotbugs_ignore.xml, and the absence of any SonarQube hook in build.gradle / code/gradle/ / GitHub Actions). That way a future Sonar pass over these files sees the intent, not just the absence.
• Kept the rest of 5353c0a intact: unused Collectors import, .toList(), pattern-matching instanceof, Class::isInstance / Class::cast method references, the two commented-out-line deletions, the //$NON-NLS-1$; stray-; fix, the // no-op: this view is read-only on the empty setData, and the Javadoc shortening.
• The two bug-fix commits (0940b62 EDT dispatch, 0addaff deferred-commit flush) are untouched.

Compile-clean (./gradlew compileJava). New tip is b98f615978.