On Fri, 16 Jan 2026 22:22:57 GMT, Christopher Schnick <[email protected]> wrote:
>> This should improve the code quality of the class while preserving its
>> original workings
>
> Christopher Schnick has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Remove space
> - Use for loops again
> - Remove unused method
@crschnick : thanks for looking at the code and trying to make it better!
I would say one thing: I would rather have us spend time on fixing real bugs
rather than introduce unnecessary changes. A cleanup is good, but there is
always a danger of introducing a regression, so we should probably undertake it
only if
a) it's a trivial and obvious change (like the unnecessary exceptions you did
earlier) or
b) the changes are a part of some greater redesign
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
231:
> 229: return;
> 230: }
> 231: for (int i = 0; i < orderedChildren.size(); i++) {
I don't know why we need to change for forEach loop, at the expense of extra
memory allocation. the old code is perfectly fine.
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
287:
> 285: } while (bot == null || !idValid);
> 286:
> 287: bot.unref();
I am not sure of this, I'd suggest to revert.
This is a kind of change that we should not be making:
- it's very time consuming to code review
- if developer of reviewers collectively made a mistake and missed a case, it
causes regression.
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
297:
> 295: }
> 296: List<NGNode> orderedChildren = getOrderedChildren();
> 297: int n = orderedChildren.size();
please remove extra space
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
301:
> 299: return orderedChildren.get(0).hasOverlappingContents();
> 300: }
> 301: return (n > 0);
the change seems ok, but... why do you force me think?
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
452:
> 450: List<NGNode> orderedChildren = getOrderedChildren();
> 451: for (NGNode orderedChild : orderedChildren) {
> 452: child = orderedChild;
completely unnecessary: you've added unnecessary memory allocation (and
iterator).
please revert.
modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGGroup.java line
473:
> 471: clone = clone.deriveWithConcatenation(getTransform());
> 472: List<NGNode> orderedChildren = getOrderedChildren();
> 473: for (final NGNode child : orderedChildren) {
please revert, for the same reason (unnecessary, extra work for computer and
people alike)
-------------
Changes requested by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/2043#pullrequestreview-3672902406
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700110052
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700119001
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700121157
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700121926
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700125746
PR Review Comment: https://git.openjdk.org/jfx/pull/2043#discussion_r2700127034
