paulk-asert commented on PR #2989: URL: https://github.com/apache/groovy/pull/2989#issuecomment-6277936793
> # Code review: [GROOVY-11434](https://issues.apache.org/jira/browse/GROOVY-11434) groovy-groovyfx > Reviewed commits: > > * `fcb630cc045950beb13d98ba4b016165db83590f` — add the optional `groovy-groovyfx` module > * `229870fa64c56ce30986170a5e38340287177612` — put the compile-only JavaFX jars on the reachability-metadata generator classpath > > This note is from reading those commits and the surrounding builder and transform code. The test suite was not re-run for this review. > > ## Recommendation > Please address the scene-root behavior before merging `fcb630cc04`. The suggested scene change is small, and it brings the implementation in line with `SceneFactory`'s own contract. The notes on numeric extensions and `@FXBindable` are smaller; they are worth doing in the same change if that stays convenient. `229870fa64` looks right as it stands. > > ## Design that fits the codebase > The module follows the `SwingBuilder` shape in the right places. > > * `SceneGraphBuilder` registers one reflective `NodeFactory` for the common node, and a separate factory only where construction itself differs: stage, scene, charts, table columns, and `bind`. > * Child placement stays in `FXChildren`. That decision depends on the parent, so one parent dispatch is easier to follow than a factory per control. > * `FXRuntime` names no JavaFX type, so a missing runtime can be reported before those classes are linked. > * Toolkit startup distinguishes "already running" from "already shut down". > * JavaFX remains compile-only and is not bundled. The GPL-2.0-with-Classpath-Exception dependency stays off the runtime classpath. > * No new source file approaches a thousand lines. `SceneGraphBuilder` is 544 lines. > > Please keep that split. Moving child placement back into per-control factories would add types without removing rules. > > ## Scene root is chosen before the sibling list is known > `SceneFactory` documents this rule: the first nested `Parent` becomes the scene root; when the first child is not a `Parent`, or when further children follow, those children belong to a `Group` root. > > `setChild` applies a different rule. On the first `Parent` it clears `groovyfx.placeholderRoot` and replaces `scene.root` immediately. A later sibling is then appended to that root when the root is a `Group` or a `Pane`. `VBox`, `HBox`, `BorderPane`, and `GridPane` are all `Pane`s, so this builder nests the second child inside the first: > > ```groovy > scene { > vbox { label('a') } > label('b') > } > ``` > > `label('b')` becomes a child of the `vbox`, beside `'a'`. Two panes, such as `vbox` followed by `hbox`, nest the second pane inside the first. A later sibling throws only when the replacement root is neither a `Group` nor a `Pane`. > > `setChild` sees one child at a time, so it cannot know whether another sibling will follow. The swap in `setChild` therefore cannot implement the documented rule. `onNodeCompleted` already runs after the whole body. > > Suggested behavior: > > 1. When no explicit `root:` is given, keep the placeholder `Group` for the entire body and add every nested `Node` to it. > 2. In `onNodeCompleted`, if that group contains exactly one child and that child is a `Parent`, remove the child from the group and then set it as `scene.root`. A node cannot have two parents, so the removal has to happen first. > 3. When the caller passed `root:`, keep adding children to that root, and skip the promotion. > > That leaves one completion rule in place of the mid-build swap, the "append to a pane" branch, and the placeholder flag being cleared on the first parent. > > Please add tests for a `vbox` followed by a `label`, and for two panes. In both cases the children should be siblings under a `Group`. The current tests cover a single `vbox` (`buildsAStageSceneAndControls`) and rectangles only (`sceneWithoutParentRootGetsAGroup`). They do not cover a `Parent` with a following sibling. > > ## Numeric extensions repeat one type dispatch > `FXExtensions` uses the same ladder in seven methods: `Integer`, `Short`, or `Byte`, then `Long`, then `Float`, otherwise `doubleValue()`. The ladder appears in `plus`, `minus`, and `div` for a constant on the right, and in `plus`, `minus`, `multiply`, and `div` for a constant on the left. > > The public overloads need to remain. Extension methods are selected by signature. The bodies can share one private method that accepts the four primitive operations (`int`, `long`, `float`, and `double`). `negative`, and the overloads that only forward to `add` or `concat`, are already direct. > > That ladder is also the numeric policy. `BigDecimal` and `BigInteger` match none of the branches, so both become `doubleValue()`. An expression such as `1.5g + property` therefore yields a `double` binding. Please make that choice once, in the shared helper: either call the `Bindings` overload that preserves the operand type, or reject a non-primitive `Number` with an exception that states the conversion. > > ## `@FXBindable` maps types by simple name after resolution > `FXPropertyTypes.forType` classifies a property by its simple name. That is appropriate for `FXBindableASTStubber`, which runs at `CONVERSION`, before names are resolved. `FXBindableASTTransformation` runs at `CANONICALIZATION`, after resolution. Please map resolved types there from the resolved `ClassNode` (`ClassHelper` primitives, `java.lang.String`, `ObservableList`, `ObservableMap`, and `ObservableSet`), and keep the simple-name table in the stubber. > > With the current shared function, any class whose simple name is `String` becomes a `StringProperty`. The annotation contract lists `boolean`, `int`, `long`, `float`, `double`, `String`, and their wrappers. `forType` also maps `short`, `byte`, and `char` to `IntegerProperty`, because those names are in `INTEGER_NAMES`. Storing a `char` as an `int` should be stated in that contract, or the mapping should stop. > > The generated `nameProperty()` method publishes the property with a check-then-act on a non-volatile field: if `name$property` is null, it assigns a new `Simple*Property`. Two threads can each construct a property, and the write that loses drops listeners attached to the discarded instance. A model filled off the JavaFX thread and then bound on it is a normal use of this annotation. Please make the property field volatile and publish it under synchronization, or generate the accessor as a synchronized method. The getter's read of `name$property == null ? name : name$property.get()` has the same visibility gap. > > There is also no joint-compilation test showing that a Java caller can see `nameProperty()` through the stub. The stubber exists for that phase. > > ## Smaller contracts > `XYChartFactory.newInstance` retains a positional value only when that value is already an instance of the chart class. A call such as `lineChart('Sales')` drops `'Sales'`. `StageFactory`, `SceneFactory`, and `NodeFactory` either use a positional value or reject it. Please reject a non-chart positional value the same way. > > `NodeFactory.construct` catches every `GroovyRuntimeException` from `invokeConstructorOf`. A constructor that runs and throws is reported as an `InvokerInvocationException`, which is a `GroovyRuntimeException`. If the bean also has a `text` property and the value is a `CharSequence`, that failure is discarded and a no-argument instance receives `text`. Please catch `MissingMethodException` for the text fallback, and let a constructor that has already run propagate its failure. > > ## Classpath fix > `229870fa64` is the right fix, and it does not need a redesign. `nativeImageMetadata` is a `JavaExec` task whose classpath is `sourceSets.main.runtimeClasspath`. That classpath omits `compileOnly` and `compileOnlyApi`. `FXExtensions` names `NumberBinding` in its signatures, so `Class.getMethods()` needs the JavaFX jars. Appending `configurations.compileClasspath` adds the configuration that contains them. Thanks for the careful read, Daniel. Addressed in 16f8cff466; notes on each point below, including a couple where I went a different way. **Scene root.** Agreed, that was a real bug and the test gap you spotted was exactly where it hid. `setChild` now only ever appends to the current root (placeholder `Group`, or an explicit `root:`), and `onNodeCompleted` promotes a sole nested `Parent` once the body is complete, detaching it from the group first. The Javadoc now states the rule, including that `scene.root` is the placeholder while the body is still being built. Added the two tests you asked for (`vbox` followed by `label`; two panes) plus one for an explicit `root:`; both new sibling tests fail against the old factory. **`XYChartFactory` positional value.** Now rejected with the same wording as `SceneFactory` (`The value of 'lineChart' must be a LineChart. Found: java.lang.String`). **`NodeFactory` constructor catch.** Agreed the blanket catch was wrong, but catching `MissingMethodException` wouldn't work: when no constructor matches, `MetaClassImpl` throws a plain `GroovyRuntimeException("Could not find matching constructor…")`, not a `MissingMethodException`, so the text fallback would never trigger. I've made the fallback a pre-check instead: a `CharSequence` value goes to the no-arg constructor + `text` property only when the bean has no constructor accepting a `String`; everything else calls `invokeConstructorOf` with no catch, so a constructor that runs and fails reports its own failure, and a missing constructor surfaces Groovy's own message (wrapped by `FactoryBuilderSupport` as before). `NodeFactoryTest` covers the fallback, a `GroovyRuntimeException` from a `String` constructor propagating, and the no-match case. **Numeric constants.** I've documented the policy (class Javadoc and the adoc) rather than changing it, because neither suggested alternative is available: JavaFX only has `int`/`long`/`float`/`double` overloads, so there's no "preserve the operand type" call for `BigDecimal`/`BigInteger`; and rejecting non-primitive `Number`s would break the most common case, since Groovy's decimal literals are `BigDecimal` (`width * 0.5` would throw). Going to `double` for both is the only sensible mapping; it's now stated. I left the seven overload bodies as they are: a shared helper taking four lambdas per call didn't read better than the four-line bodies. **`@FXBindable` type mapping by simple name.** This one is deliberate (the `FXPropertyTypes` Javadoc notes it): the stubber at CONVERSION and the transform at CANONICALIZATION must give the same answer, or a user class that happens to be called `String` would get a stub returning `StringProperty` and a class returning `ObjectProperty<…>`, which javac would compile against and then fail to link. Consistency across the two phases matters more than exactness in one of them, and the colliding case is contrived. **`short`/`byte`/`char`.** The contract now documents `short` and `byte` widening to `IntegerProperty`. I've dropped `char`: it now maps to `ObjectProperty<Character>` like any other unlisted type, which round-trips cleanly rather than storing a char as an int. Test added for all three. **Thread-safety of `nameProperty()`.** I'd rather keep the unsynchronized lazy check-then-act: it's the standard JavaFX lazy-property idiom (JavaFX's own `Node` and `Control` properties are written exactly this way), and JavaFX properties and their listener lists aren't thread-safe in any case, so a synchronized accessor would cost every call without making off-thread binding safe. The scenario you describe (fill the model off the FX thread, bind on it) already works: before the first `nameProperty()` call the setter writes the plain field, and handing over via `Platform.runLater` gives the happens-before. Happy to add a sentence to the Javadoc saying the usual JavaFX thread-confinement rules apply if you think it's worth calling out. **Joint compilation.** Fair gap. Added `FXBindableJointCompilationTest`, which compiles a class-level `@FXBindable` class together with a Java caller through `JavaAwareCompilationUnit` with `keepStubs`, asserts the stub declares `StringProperty nameProperty()` / `IntegerProperty ageProperty()`, then loads and runs the Java class. **Classpath fix.** No change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
