[
https://issues.apache.org/jira/browse/GROOVY-11434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128906#comment-18128906
]
ASF GitHub Bot commented on GROOVY-11434:
-----------------------------------------
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.
> Include GroovyFx in Apache Groovy
> ---------------------------------
>
> Key: GROOVY-11434
> URL: https://issues.apache.org/jira/browse/GROOVY-11434
> Project: Groovy
> Issue Type: New Feature
> Components: Swing
> Affects Versions: 4.0.22
> Reporter: Pruteanu Dragos
> Assignee: Paul King
> Priority: Major
> Fix For: 7.x
>
>
> Please include the project
> [GroovyFX|[http://groovyfx.org|http://groovyfx.org/]] into the Apache Groovy
> repository. The public repository is
> [here|https://github.com/groovyfx-project/groovyfx]. The project is very
> nice, something similar to groovy-swing. There has been no updates to that
> project, and there are compatibility issues with latest OpenJDKs.
> JavaFX is the highest standard regarding User Interfaces. The project is very
> nice and there are users interested in it.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
