[
https://issues.apache.org/jira/browse/GROOVY-11434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128813#comment-18128813
]
ASF GitHub Bot commented on GROOVY-11434:
-----------------------------------------
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.
> 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)
