daniellansun commented on PR #2989:
URL: https://github.com/apache/groovy/pull/2989#issuecomment-6270524639
# Code review: 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.
--
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]
