RFR: 8366739: ToolBar: overflow menu with fractional scale (2)

[Cormac Redmond]

On Sun, 21 Dec 2025 22:08:33 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> An example of length rounding due to "-=" operations (again, at 1.25 scale), when overflow should show:
>> 
>> <img width="855" height="247" alt="image" src="https://github.com/user-attachments/assets/aca455ab-0d32-4e46-9868-78d786cd4a7b" />
>> 
>> I can't (easily) get it to create any actually problems for me, but the question remains, should be re-snap it also?
>> 
>> If it's not (demonstrably) broken, don't fix it, maybe.
>
> Yeah, that's going to cause problems at some point.  It must be resnapped when doing comparisons with it.

@hjohn, thanks. I began to fix "length" there too at source (i.e., after the length manipulations, which I think is the place to do it). 

However, while doing so, I asked myself if there's any actual need to wrap a snapSize in a snapPosition in the first place?

For example, the original fix I added earlier for "x", was like this:

`x = snapPositionY(x + snapSizeY(node.prefHeight(-1)) + getSpacing());`

...but could it not simply be:

`x = snapSizeY(x + node.prefHeight(-1) + getSpacing());` ...?

This seems to be how the ToolBar length is calculated:


  private double getToolbarLength(ToolBar toolbar) {
        if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
            return snapSizeY(snapSizeY(toolbar.getHeight()) - snappedTopInset() - snappedBottomInset() + getSpacing());
        } else {
            return snapSizeX(snapSizeX(toolbar.getWidth()) - snappedLeftInset() - snappedRightInset() + getSpacing());
        }
    }


If I am correct, to fix the latest "length" problem, I am thinking I can simple change it from this problematic line:

`length -= snapSizeY(overflowMenu.prefHeight(-1));`

to this:

`length = snapSizeY(length - overflowMenu.prefHeight(-1));`

I could also remove the snapPosition wrapper from my original fix for "x". I think leaving it there implies it has a required purpose (but it may not).

So overall, I am proposing doing the below to changes, instead.

New fix for `length`, change this:

        if (newOverflowNodeIndex < getSkinnable().getItems().size()) {
            if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
                length -= snapSizeY(overflowMenu.prefHeight(-1));
            } else {
                length -= snapSizeX(overflowMenu.prefWidth(-1));
            }
            length -= getSpacing();
            newOverflowNodeIndex = getOverflowNodeIndex(length);
        }



... to this:


  if (newOverflowNodeIndex < getSkinnable().getItems().size()) {
            length -= getSpacing(); // MOVED
            if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
                length = snapSizeY(length - overflowMenu.prefHeight(-1)); // SIMPLIFIED
            } else {
                length = snapSizeX(length - overflowMenu.prefWidth(-1)); // SIMPLIFIED
            }
            newOverflowNodeIndex = getOverflowNodeIndex(length);
        }


And, also reverting my earlier fix for `x`, from this:

  if (node.isManaged()) {
                if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
                    x = snapPositionY(x + snapSizeY(node.prefHeight(-1)) + getSpacing()); // NECESSARY?
                } else {
                    x = snapPositionX(x + snapSizeX(node.prefWidth(-1)) + getSpacing()); // NECESSARY?
                }
            }


... to this:


            if (node.isManaged()) {
                if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
                    x = snapSizeY(x + node.prefHeight(-1) + getSpacing()); // SIMPLIFIED
                } else {
                    x = snapSizeX(x + node.prefWidth(-1) + getSpacing()); // SIMPLIFIED
                }
            }


Test suite and manual testing isn't finding any issues.

What do you think?

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/2016#discussion_r2639607102