On Wed, 30 Jul 2025 16:34:22 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Andy Goryachev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains two additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into 8364049.toolbar
>> - snap length
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1856#discussion_r2243445856
