Re: RFR: 8344372: Setting width for TRANSPARENT Stage -> gtk_window_resize: assertion 'height > 0' [v3]
> The bug happened when setting only width or height (not both) on a Stage that > was oriented by the view size. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Fix test package - Changes: - all: https://git.openjdk.org/jfx/pull/1654/files - new: https://git.openjdk.org/jfx/pull/1654/files/9dd6961f..e74b18c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1654&range=02 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1654&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1654.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1654/head:pull/1654 PR: https://git.openjdk.org/jfx/pull/1654
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. Thanks John. Because it's the weekend I'll wait until Monday evening with this. - PR Comment: https://git.openjdk.org/jfx/pull/1653#issuecomment-2508938693
RFR: 8344372: Setting width for TRANSPARENT Stage -> gtk_window_resize: assertion 'height > 0'
The bug happened when setting only width or height (not both) on a Stage that was oriented by the view size. - Commit messages: - Fix 8344372 Changes: https://git.openjdk.org/jfx/pull/1654/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1654&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8344372 Stats: 118 lines in 2 files changed: 118 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1654.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1654/head:pull/1654 PR: https://git.openjdk.org/jfx/pull/1654
Re: RFR: 8344372: Setting width for TRANSPARENT Stage -> gtk_window_resize: assertion 'height > 0' [v2]
> The bug happened when setting only width or height (not both) on a Stage that > was oriented by the view size. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Fix test class name - Changes: - all: https://git.openjdk.org/jfx/pull/1654/files - new: https://git.openjdk.org/jfx/pull/1654/files/4e36b428..9dd6961f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1654&range=01 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1654&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1654.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1654/head:pull/1654 PR: https://git.openjdk.org/jfx/pull/1654
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. @azvegint Can you take a quick look at this since you added the Dimension class as part of your initial Wayland Robot fix? - PR Comment: https://git.openjdk.org/jfx/pull/1653#issuecomment-2508969153
Re: RFR: 8344899: Map RT-nnnn bug IDs to JDK-mmmmmmm in JavaFX sources [v4]
On Fri, 29 Nov 2024 09:24:31 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 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 Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1648#pullrequestreview-2470908018
Re: Possible mistakes in com.sun.javafx.geom.AreaOp
I agree with your type analysis. However, I'm hesitant to change the logic
in regarding 'numedges' without someone who is familiar with the domain
taking a look; these sorts of computations often have non-obvious sides to
them.
I also think that the class was copied. The raw types usage suggests a 1.4
era. Vector itself suggests even earlier times, perhaps AWT. If the code
doesn't require thread synchronization, and I don't think it does, using
List could even speed it up a bit.
Do you have an idea about the needless 'toArray' calls? The return is not
captured and it doesn't have side effects. Maybe to check the types?
On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx
wrote:
> 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:
>
> public Vector calculate(Vector left, Vector right) {
>
> Vector edges = new Vector<>();
>
> addEdges(edges, left, AreaOp.CTAG_LEFT);
>
> addEdges(edges, right, AreaOp.CTAG_RIGHT);
>
> Vector curves = pruneEdges(edges);
>
> if (false) {
>
> System.out.println("result: ");
>
> int numcurves = curves.size();
>
> Curve[] curvelist = (Curve[]) edges.toArray(new Curve[numcurves]);
>
> for (int i = 0; i < numcurves; i++) {
>
> System.out.println("curvelist["+i+"] = "+curvelist[i]);
>
> }
>
> }
>
> return curves;
>
> }
>
> 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:
>
> private Vector pruneEdges(Vector edges) {
>
> int numedges = edges.size();
>
> if (numedges < 2) {
>
> return edges;
>
> }
>
> 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:
>
> private Vector pruneEdges(Vector edges) {
>
> int numedges = edges.size();
>
> if (numedges < 2) {
>
> return numedges == 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 'to
Re: Possible mistakes in com.sun.javafx.geom.AreaOp
Probably derived from java.awt.Area:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java
Le sam. 30 nov. 2024, 17:59, Nir Lisker a écrit :
> I agree with your type analysis. However, I'm hesitant to change the logic
> in regarding 'numedges' without someone who is familiar with the domain
> taking a look; these sorts of computations often have non-obvious sides to
> them.
>
> I also think that the class was copied. The raw types usage suggests a 1.4
> era. Vector itself suggests even earlier times, perhaps AWT. If the code
> doesn't require thread synchronization, and I don't think it does, using
> List could even speed it up a bit.
>
> Do you have an idea about the needless 'toArray' calls? The return is not
> captured and it doesn't have side effects. Maybe to check the types?
>
> On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx
> wrote:
>
>> 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:
>>
>> public Vector calculate(Vector left, Vector right) {
>>
>> Vector edges = new Vector<>();
>>
>> addEdges(edges, left, AreaOp.CTAG_LEFT);
>>
>> addEdges(edges, right, AreaOp.CTAG_RIGHT);
>>
>> Vector curves = pruneEdges(edges);
>>
>> if (false) {
>>
>> System.out.println("result: ");
>>
>> int numcurves = curves.size();
>>
>> Curve[] curvelist = (Curve[]) edges.toArray(new Curve[numcurves]);
>>
>> for (int i = 0; i < numcurves; i++) {
>>
>> System.out.println("curvelist["+i+"] = "+curvelist[i]);
>>
>> }
>>
>> }
>>
>> return curves;
>>
>> }
>>
>> 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:
>>
>> private Vector pruneEdges(Vector edges) {
>>
>> int numedges = edges.size();
>>
>> if (numedges < 2) {
>>
>> return edges;
>>
>> }
>>
>> 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:
>>
>> private Vector pruneEdges(Vector edges) {
>>
>> int numedges = edges.size();
>>
>> if (numedges < 2) {
>>
>> return numedges == 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 'pru
Re: Possible mistakes in com.sun.javafx.geom.AreaOp
You're right, these geometry classes were copied from AWT. Here is the
offending AreOp:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/geom/AreaOp.java
.
They generified their code at some point like John did. They also just
create a new vector in the 'numedges' check. I wonder why they kept Vector
though.
On Sat, Nov 30, 2024 at 7:43 PM Laurent Bourgès
wrote:
> Probably derived from java.awt.Area:
>
> https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java
>
> Le sam. 30 nov. 2024, 17:59, Nir Lisker a écrit :
>
>> I agree with your type analysis. However, I'm hesitant to change the
>> logic in regarding 'numedges' without someone who is familiar with the
>> domain taking a look; these sorts of computations often have non-obvious
>> sides to them.
>>
>> I also think that the class was copied. The raw types usage suggests a
>> 1.4 era. Vector itself suggests even earlier times, perhaps AWT. If the
>> code doesn't require thread synchronization, and I don't think it does,
>> using List could even speed it up a bit.
>>
>> Do you have an idea about the needless 'toArray' calls? The return is not
>> captured and it doesn't have side effects. Maybe to check the types?
>>
>> On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx
>> wrote:
>>
>>> 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:
>>>
>>> public Vector calculate(Vector left, Vector right)
>>> {
>>>
>>> Vector edges = new Vector<>();
>>>
>>> addEdges(edges, left, AreaOp.CTAG_LEFT);
>>>
>>> addEdges(edges, right, AreaOp.CTAG_RIGHT);
>>>
>>> Vector curves = pruneEdges(edges);
>>>
>>> if (false) {
>>>
>>> System.out.println("result: ");
>>>
>>> int numcurves = curves.size();
>>>
>>> Curve[] curvelist = (Curve[]) edges.toArray(new Curve[numcurves]);
>>>
>>> for (int i = 0; i < numcurves; i++) {
>>>
>>> System.out.println("curvelist["+i+"] = "+curvelist[i]);
>>>
>>> }
>>>
>>> }
>>>
>>> return curves;
>>>
>>> }
>>>
>>> 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:
>>>
>>> private Vector pruneEdges(Vector edges) {
>>>
>>> int numedges = edges.size();
>>>
>>> if (numedges < 2) {
>>>
>>> return edges;
>>>
>>> }
>>>
>>> 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:
>>>
>>> private Vector pruneEdges(Vector edges) {
>>>
>>> int numedges = edges.size();
>>>
>>> if (numedges < 2) {
>>>
>>> return numedges == 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:
>>>
>>>
