On Tue, 23 Dec 2025 14:12:37 GMT, John Hendrikx <[email protected]> wrote:
>> Cormac Redmond has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8366739: minor test comment improvement
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java
> line 702:
>
>> 700: } else {
>> 701: x = snapPositionX(x + snapSizeX(node.prefWidth(-1))
>> + getSpacing());
>> 702: length = snapPositionX(length);
>
> Two things here:
>
> - You're snapping `length` each time through the loop, I would just put that
> outside the loop
> - You're modifying an argument to the method; it might be clearer to use
> `snappedLength` here (but that's just me, I don't like modifying arguments to
> methods).
>
> If you're worried about the `getSkinnable` and `getOrientation` call, these
> should be constant through-out this loop (so that can be factored out as well
> if you want).
I definitely did not mean to push that pointless re-calculation, not sure how I
didn't spot it.
Agree on modifying the argument, I'd normally make them final. I've use
snappedLength as suggested.
Regarding the factoring out of getOrientation(), I can't see a way to do that
without introducing a boolean and using that instead (or create a Function that
takes an orientation and calls snapPositionX or Y accordingly, which is
probably not a good idea).
I added a boolean...maybe that's what you meant.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2643412455
