On Tue, 23 Dec 2025 13:18:27 GMT, Cormac Redmond <[email protected]> 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
