On Wed, 30 Jul 2025 17:42:54 GMT, Andy Goryachev <[email protected]> wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ToolBarSkin.java
>> line 692:
>>
>>> 690: */
>>> 691: private int getOverflowNodeIndex(double length) {
>>> 692: if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
>>
>> Snapping the input argument of a method like `getOverflowNodeIndex` doesn't
>> seem right to me, because the length is computed incorrectly in the first
>> place. Have you considered fixing the snapping in `getToolbarLength`? This
>> also solves the problem.
>
> Yes, but I wanted to do a very localized fix, to be included in jfx25. There
> are many places where we should be more careful with snapping mentioned in
> description, and I suspect even more elsewhere.
>
> I am still not sure what would be the best approach to address them all. We
> could probably create an umbrella task and fix individual controls and
> containers (maybe even start with containers, in continuation of John's work
> with HBox and VBox).
>
> I think this PR is still good, because a) it provides a fix for the specific
> issue and b) minimizes regression, but I agree with you that a comprehensive
> fix is needed.
>
> What do you think?
I understand that you want this to be a localized fix. I'm just proposing to
change `getToolbarLength` instead, which is even less changed code than your
change in `getOverflowNodeIndex`, and seems to be addressing the source of the
incorrect length instead.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1856#discussion_r2243493210
