Re: RFR: 8344899: Map RT-nnnn bug IDs to JDK-mmmmmmm in JavaFX sources [v4]
> This PR changes all `RT-` references to `JDK-XXX`. > It also removes all `http://javafx-jira.kenai.com/browse/` occurrences. > > As discussed, this will help a lot when looking up old issues, especially > since not everybody know how to do a lookup with the `RT-` number in the > JIRA. > > Thanks to @kevinrushforth who provided the mapping! > I wrote a small Java program that replaces all the occurrences. This includes > the following files: > - `.java, .css, .m, .h, .cc, .vert, .jsl, .c, .cpp` Marius Hanl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' of https://github.com/openjdk/jfx into 8344899-rt-to-jdk # Conflicts: #modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.m - 8344899: Revert accidentally renamed test comments - 8344899: Remove occurrences of old JIRA tickets Including: - JFXC - DTL - JMY - 8344899: Map RT- bug IDs to JDK-mmm in JavaFX sources - Changes: https://git.openjdk.org/jfx/pull/1648/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1648&range=03 Stats: 1012 lines in 361 files changed: 0 ins; 4 del; 1008 mod Patch: https://git.openjdk.org/jfx/pull/1648.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1648/head:pull/1648 PR: https://git.openjdk.org/jfx/pull/1648
Re: RFR: 8344899: Map RT-nnnn bug IDs to JDK-mmmmmmm in JavaFX sources [v3]
On Thu, 28 Nov 2024 12:52:33 GMT, Marius Hanl wrote: >> This PR changes all `RT-` references to `JDK-XXX`. >> It also removes all `http://javafx-jira.kenai.com/browse/` occurrences. >> >> As discussed, this will help a lot when looking up old issues, especially >> since not everybody know how to do a lookup with the `RT-` number in the >> JIRA. >> >> Thanks to @kevinrushforth who provided the mapping! >> I wrote a small Java program that replaces all the occurrences. This >> includes the following files: >> - `.java, .css, .m, .h, .cc, .vert, .jsl, .c, .cpp` > > Marius Hanl has updated the pull request incrementally with one additional > commit since the last revision: > > 8344899: Revert accidentally renamed test comments Solved the merge conflict with `GlassView3D.m`. - PR Comment: https://git.openjdk.org/jfx/pull/1648#issuecomment-2507397413
RFR: 8345261: Refactor the Dimension2D classes
A small refactoring of the Dimension classes. * `com.sun.javafx.geom.Dimension` was removed and its uses were replaced by `com.sun.javafx.geom.Dimension2D`. * `com.sun.javafx.geom.Dimension2D` became a record. * `javafx.geometry.Dimension2D`: fields became `final`. I'm not sure we need the implementation class at all considering we are free to use the public one. - Commit messages: - Revert static import wildcard use - Initial commit Changes: https://git.openjdk.org/jfx/pull/1653/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1653&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8345261 Stats: 93 lines in 7 files changed: 5 ins; 70 del; 18 mod Patch: https://git.openjdk.org/jfx/pull/1653.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1653/head:pull/1653 PR: https://git.openjdk.org/jfx/pull/1653
Re: Possible mistakes in com.sun.javafx.geom.AreaOp
Hi Nir,
I encountered that class before while doing raw warning clean-ups in
graphics (which were never integrated).
The problem IMHO is in the assignment in `calculate`:
edges = pruneEdges(edges);
This assignment is both confusing and unnecessary, and violates the
principle of re-using variables for different purposes. Method
pruneEdges must return CURVES (the name even implies it -- remove
edges), and the debug code in this block even assumes it does (it
assigned `numcurves` to `edges.size()` then assumes that it contains
curves when converting to the array format):
Rewrite the code as follows and it will be much clearer:
publicVectorcalculate(Vector left, Vector right) {
Vector edges = newVector<>();
addEdges(edges, left, AreaOp.CTAG_LEFT);
addEdges(edges, right, AreaOp.CTAG_RIGHT);
Vector curves = pruneEdges(edges);
if(false) {
System.out.println("result: ");
intnumcurves = curves.size();
Curve[] curvelist = (Curve[]) edges.toArray(newCurve[numcurves]);
for(inti = 0; i < numcurves; i++) {
System.out.println("curvelist["+i+"] = "+curvelist[i]);
}
}
returncurves;
}
Then the initial code in `pruneEdges` is just plain wrong, and seems to
be an attempt at optimization done incorrectly by returning an existing
vector to save having to allocate a new one:
privateVectorpruneEdges(Vector edges) {
intnumedges = edges.size();
if(numedges < 2) {
returnedges;
}
Method pruneEdges however is supposed to return a minimum set of curves
that enclose an area. A single edge can't contribute any area. I'm
pretty sure that if you change the check to `numedges < 1` that if the
remaining code runs normally it would also come to that conclusion (it
would return an empty vector of curves). So this "optimization" here is
doing the wrong thing by potentially returning a single edge instead of
always returning an empty curve vector.
Now, I'm pretty sure we'll never see this case in practice, due to other
checks being done (specifically the `getOrder() > 0` check when adding
edges) and the likely fact that there is always going to be a minimum of
2 curves being passed to `calculate. Adding an assert or just modifying
the code, and then running all tests may help verify this. I believe
however that the code needs to be this:
privateVectorpruneEdges(Vector edges) {
intnumedges = edges.size();
if(numedges < 2) {
returnnumedges == 0 ? (Vector)(Vector)edges : new Vector(); //
as a single edge can't encompass any area, a single edge also results in
no curves being returned
}
The reason why we do need to return a mutable Vector here is because the
classes using the return value of calculate assume that it is writable
and doesn't need copying. The ugly double cast of edges is relatively
contained -- it is only casted when empty (to avoid creating another
vector instance), and it was created by this class so its under
control. Always returning `new Vector()` would also work, but it may
perform a tiny bit slower in some cases (probably not measurable IMHO).
I suspect the entire class is copied from somewhere, as at the time
JavaFX was written using Vector while at the same time trying to do
these kinds of optimizations is odd to say the least.
--John
On 30/11/2024 00:12, Nir Lisker wrote:
> I came across a potential mistake in the
> class com.sun.javafx.geom.AreaOp. It uses raw Vector types and while
> trying to add generic parameters there for type safety, I got some
> conflicts.
>
> In the method AreaOp::calculate, the arguments should be Vector
> and the return type should also be Vector, but it returns a
> Vector called 'edges'. 'edges' is passed to the 'addEdges' method that
> should take Vector and Vector. This means that 'edges' is
> a Vector and not Vector. Already a conflict.
> Then it is passed to 'pruneEdges', which, if it takes and returns
> Vector, runs into a conflict with the return type: 'Vector ret'
> has Curve added to it. If I try to return Vector instead, then
> in the 'numedges < 2' check the input Vector can't be returned unless
> it's also Vector, which again causes a conflict in 'calculate'.
>
> There are also 'toArray' calls that seem to not do anything, like in
> 'resolveLinks' (line 454) and in 'finalizeSubCurves' (429).
>
> Can anyone who knows this part of the code take a look?
>
> - Nir
Re: RFR: 8345261: Refactor the Dimension2D classes
On Fri, 29 Nov 2024 17:00:42 GMT, Nir Lisker wrote: > A small refactoring of the Dimension classes. > > * `com.sun.javafx.geom.Dimension` was removed and its uses were replaced by > `com.sun.javafx.geom.Dimension2D`. > * `com.sun.javafx.geom.Dimension2D` became a record. > * `javafx.geometry.Dimension2D`: fields became `final`. > > I'm not sure we need the implementation class at all considering we are free > to use the public one. Looks good - Marked as reviewed by jhendrikx (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1653#pullrequestreview-2470718984
Possible mistakes in com.sun.javafx.geom.AreaOp
I came across a potential mistake in the class com.sun.javafx.geom.AreaOp. It uses raw Vector types and while trying to add generic parameters there for type safety, I got some conflicts. In the method AreaOp::calculate, the arguments should be Vector and the return type should also be Vector, but it returns a Vector called 'edges'. 'edges' is passed to the 'addEdges' method that should take Vector and Vector. This means that 'edges' is a Vector and not Vector. Already a conflict. Then it is passed to 'pruneEdges', which, if it takes and returns Vector, runs into a conflict with the return type: 'Vector ret' has Curve added to it. If I try to return Vector instead, then in the 'numedges < 2' check the input Vector can't be returned unless it's also Vector, which again causes a conflict in 'calculate'. There are also 'toArray' calls that seem to not do anything, like in 'resolveLinks' (line 454) and in 'finalizeSubCurves' (429). Can anyone who knows this part of the code take a look? - Nir
