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

[John Hendrikx]

On Tue, 23 Dec 2025 13:18:27 GMT, Cormac Redmond <duke at openjdk.org> wrote:

>> To add a bit more to this, I think there are a few general rules when it comes to snapping:
>> 
>> ## 1. Snap External Values Early
>> - Each node has its own snap setting.
>> - When using values from other nodes (e.g., `prefHeight`), **convert them as early as possible into the current node’s snap space** to ensure consistency in layout calculations.
>> 
>> ## 2. Use Ceiling for Size Values from Other Nodes
>> - External values are usually **sizes**.
>> - It is important to fully respect them—for example, a `Text` node requiring 11.5774 pixels should not be rounded down to 11.5, as that could trigger ellipsis or clipping.
>> - When bringing these values into your snap space, use a **ceiling function** (`snapSize`) to guarantee the content fits.
>> 
>> ## 3. Use Rounding for Internal Calculations
>> - When combining values that **belong to your current node** (spacing, margins, gaps) or have been imported already (by snapping an external source), use **rounding functions** (`snapPosition`, `snapSpace`).
>> - This prevents accumulation of extra pixels that can cause visual discontinuities.
>> 
>> ## 4. Current Node Sizes
>> - Any sizes that are important for content layout (like text widths or heights) should be snapped with **ceiling** (`snapSize`).
>> - Other quantities such as spacing or gaps do not need ceiling; small discrepancies here are visually insignificant and won’t cause clipping.
>> - Apply ceiling **once** when needed; repeated ceiling calls can lead to overestimation of total sizes.
>> 
>> ---
>> 
>> ## Summary
>> 
>> | Value Type                     | Snap Function |
>> |--------------------------------|---------------|
>> | External node sizes             | `ceil` (`snapSize`) |
>> | Internal sums / positions       | `round` (`snapPosition`, `snapSpace`) |
>> | Current node sizes (important)  | `ceil` (`snapSize`), once only |
>> | Internal spacings / gaps        | `round` or unsnapped |
>
> Thanks @hjohn. That all makes sense. I think I am understanding the purpose of these now (have been shooting from the hip a bit).
> 
> Side not: would be good to get the above message somewhere more visible (not sure where though...), maybe a JavaDoc in Region?
> 
> Anyway, after all that, I've pushed what might hopefully be the final change, and it's confined to just this code in getOverflowNodeIndex()(:
> 
> 
>             if (node.isManaged()) {
>                 if (getSkinnable().getOrientation() == Orientation.VERTICAL) {
>                     x = snapPositionY(x + snapSizeY(node.prefHeight(-1)) + getSpacing());
>                     length = snapPositionY(length);
>                 } else {
>                     x = snapPositionX(x + snapSizeX(node.prefWidth(-1)) + getSpacing());
>                     length = snapPositionX(length);
>                 }
>             }
> 
> 
> I decided to fix `length` within getOverflowNodeIndex() itself rather than in the calling method (i.e., don't assume it comes pre-snapped). I think since getOverflowNodeIndex() is doing the comparison, it's its responsibility.

Side note: yeah, it probably deserves a place somewhere in `Region`.

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

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