Skip to content

Fix NPE on load and improve row clicking#7590

Open
karianna wants to merge 2 commits into
PCGen:masterfrom
karianna:master
Open

Fix NPE on load and improve row clicking#7590
karianna wants to merge 2 commits into
PCGen:masterfrom
karianna:master

Conversation

@karianna

@karianna karianna commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

@Vest

Vest commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Independent review — verdict: merge with nits

NPE fix looks structurally correct. Splitting JFXPanelFromResource (Swing-embedded, JFXPanel auto-inits FX runtime) from PanelFromResource (pure FXML→Stage, no embedded peer) is the right shape. All showAsStage/showAndBlock call sites are migrated; remaining JFXPanelFromResource references in the tree (InfoGuidePane, DataInstaller html pane, the various InfoTab html panes, HtmlSheetSupport, ConvertedJavaFXPanel, PCGenPreferencesModel) are all genuine Swing-embed uses — verified.

Findings

None of these block merge.

  1. AGENTS.md version table is stale on day one. This is a bit awkward given the PR's own preamble says "keep this file up to date":

    • Gradle 9.5.0 → actual 9.5.1
    • JUnit BOM 6.0.36.1.0
    • Saxon 12.913.0
    • SpotBugs 6.5.46.5.5
    • PMD 7.21.0 / 7.24.07.25.0
    • jlink 4.0.04.0.2

    Either fix the numbers or drop the version table entirely — otherwise it'll rot fast and become a misleading reference.

  2. PanelFromResource.showAsStage is not idempotent on the same instance (code/src/java/pcgen/gui3/PanelFromResource.java:80–96). It calls fxmlLoader.load() per invocation, so calling showAsStage twice on one instance re-parses the FXML and replaces the controller. No current caller does this (every site builds a fresh instance), but worth a javadoc warning, or load-once into a Parent and build a fresh Scene per show.

  3. getController() returns null on a never-shown instance (PanelFromResource.java:64–72). Old JFXPanelFromResource loaded FXML in the constructor, so getController() was always usable. The new javadoc documents this, and no migrated site reads the controller before show* (verified for Main.loadProperties and PurchaseModeFrame.addMethodButtonActionPerformed, which reads it after showAndBlock returns — i.e. after the inline fxmlLoader.load() inside the Platform.runLater body has run). Behavior is correct, but any future caller doing new PanelFromResource(...).getController().setX(...) to pre-configure before show will silently NPE.

  4. Calculator dialog is no longer a singleton across F11 presses (PCGenActionMap.java:258–280). Worth flagging in the commit message that this is intentional: the old singleton reused a Scene across Stages, which is precisely the embedded-scene-reparenting bug being fixed. A Scene can only be attached to one Stage at a time. State loss between presses is expected and matches the existing Export-dialog pattern.

  5. .gitignore additions (.gitignore:120–122). .superset/ appears to be local tooling/IDE noise — no references in build.gradle, CI, or anywhere else. Harmless to ignore. The leading blank line above it is gratuitous; consider tightening.

  6. Javadoc nit on PanelFromResource.showAndBlock: verify the documented throw type matches what GuiAssertions.assertIsNotJavaFXThread() actually throws.

Recommendation

Merge as-is is fine; ideally fix nit #1 first (or remove the version table) since the PR explicitly establishes the rule that AGENTS.md should stay accurate. Nits 2 and 3 are good follow-up material.

(Reviewed by an LLM agent; happy to clarify any of the above.)

@Vest

Vest commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Update: ran the repro locally — PR introduces a startup regression

Followed up on my own review by actually running both the parent commit and this PR's HEAD on macOS Retina (Built-in Liquid Retina XDR, 3456×2234). Setup: moved repo config.ini aside so Main.loadProperties falls into the OptionsPathDialog flow on next launch (mimics a fresh install).

Old code (ad6faf0df6, parent of this PR) — bug reproduces

java.lang.NullPointerException: Cannot invoke "com.sun.javafx.tk.quantum.SceneState.update()" because "this.sceneState" is null
    at javafx.graphics@25.0.3/com.sun.javafx.tk.quantum.GlassScene.updateSceneState(GlassScene.java:253)
    at javafx.graphics@25.0.3/com.sun.javafx.tk.quantum.EmbeddedScene.lambda$setPixelScaleFactors$1(EmbeddedScene.java:158)
    at javafx.graphics@25.0.3/com.sun.javafx.tk.quantum.QuantumToolkit.runWithRenderLock(QuantumToolkit.java:442)
    at javafx.graphics@25.0.3/com.sun.javafx.tk.quantum.EmbeddedScene.lambda$setPixelScaleFactors$0(EmbeddedScene.java:157)
    at javafx.graphics@25.0.3/com.sun.javafx.application.PlatformImpl.lambda$runLater$0(PlatformImpl.java:424)
    ...
    at javafx.graphics@25.0.3/javafx.stage.Stage.showAndWait(Stage.java:454)
    at pcgen.gui3.JFXPanelFromResource.lambda$showAndBlock$0(JFXPanelFromResource.java:108)

Diagnosis confirmed: Stage.showAndWait enters the nested event loop, EmbeddedScene.setPixelScaleFactors runs (HiDPI scale recompute on the now-orphaned embedded scene), sceneState is null, NPE. Fires twice during dialog open and twice more during normal UI operation. JavaFX's LoggingUncaughtExceptionHandler swallows them, so the app limps along.

PR HEAD (8461bd4c68) — new regression on the same code path

java.lang.IllegalStateException: Toolkit not initialized
    at javafx.graphics@25.0.3/com.sun.javafx.application.PlatformImpl.runLater(PlatformImpl.java:408)
    at javafx.graphics@25.0.3/com.sun.javafx.application.PlatformImpl.runLater(PlatformImpl.java:403)
    at javafx.graphics@25.0.3/javafx.application.Platform.runLater(Platform.java:171)
    at pcgen.gui3.PanelFromResource.showAndBlock(PanelFromResource.java:132)
    at pcgen.system.Main.loadProperties(Main.java:265)
    at pcgen.system.Main.startupWithGUI(Main.java:185)
    at pcgen.system.Main.main(Main.java:136)

The JVM exits after the SEVERE log line — OptionsPathDialog never opens, Main.loadProperties aborts. A fresh install (no config.ini/settingsPath) cannot start the app.

Cause

JFXPanelFromResource extends JFXPanel. Instantiating JFXPanel has a critical side effect: JFXPanel.<init> calls PlatformImpl.startup(...) and bootstraps the JavaFX toolkit. The old startup path worked from the main thread — even with the NPE in HiDPI rendering — because new JFXPanelFromResource(...) had already brought up the toolkit.

The new PanelFromResource is a plain Object and does no such bootstrapping. The first Platform.runLater from main thread throws IllegalStateException: Toolkit not initialized, full stop. The PR removed the implicit init without replacing it.

Suggested fix

In Main.startupWithGUI (or wherever the first FX call may originate), explicitly initialize the toolkit before any Platform.runLater. Either:

// Option 1: cheap, well-defined
javafx.embed.swing.JFXPanel.<init>();   // discard the panel; just need the side effect

or, more cleanly:

// Option 2: explicit, no Swing dependency
javafx.application.Platform.startup(() -> { /* no-op */ });

Platform.startup is idempotent against IllegalStateException only if you guard it (it throws if already started), so Platform.isFxApplicationThread() doesn't help here — a private static volatile boolean fxStarted flag, or catching the IllegalStateException from startup, is the cleanest pattern.

Repro recipe

For anyone wanting to confirm:

mv config.ini /tmp/config.ini.bak       # forces OptionsPathDialog flow
git checkout ad6faf0df6                 # parent of this PR — old code
./gradlew run                           # see GlassScene.updateSceneState NPE in pcgen.log
# kill app
git checkout 8461bd4c68                 # this PR's HEAD
./gradlew run                           # see Toolkit not initialized in pcgen.log; app exits
mv /tmp/config.ini.bak config.ini       # restore

Reverting my earlier "merge with nits" recommendation to hold — please address the toolkit-init regression before merging. The split design is right; the implementation just needs the missing toolkit-bootstrap call.

(LLM-assisted review; happy to clarify or test a proposed fix.)

@Vest

Vest commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

@karianna my bot agrees with you. I think, you can merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants