On Mon, 10 Aug 2026 12:42:59 GMT, Nir Lisker <[email protected]> wrote:
>> You are right that the final value should be snapped, although in this case >> the error will be tiny (I think it snaps `left` and `right` so at least >> we're not summing snapped and unsnapped values here). >> >> However, in the interest of proving that this PR doesn't have any functional >> changes, I didn't do this (slight errors do make some tests fail and they'd >> need adjusment). >> >> For similar reasons I haven't switched `Math.round` to `Math.rint` in this >> PR, even though we really should do that soon (`Math.round` will mess up >> large double values, and does a conversion to `long` that we really don't >> need). > > There was a discussion about snapping only final values or also intermediate > values, but I can't recall where. Maybe it was in > https://github.com/openjdk/jfx/pull/445. I don't mind doing a 2nd pass fixing these small things. I can mark them with a TODO and then fix in another PR while adjusting the small test deviations that are likely to occur then. Things I'm already aware of: - StackPane (and BorderPane) not snapping the insets (major, adding snapped + unsnapped) - The compute methods not doing a final snap when adding several snapped values together (minor) - Some compute methods taking shortcuts assuming unbiased calculations (medium) - Use of `round` instead of `rint` which doesn't do well with values beyond the `long` range (minor) - Use of `Math.down` or `Math.ulp` in `ScaledMath.ceil` -- this should be a fixed epsilon (like 1 millionth of a pixel) (minor) I can also consolidate all the `computeChildMin/Pref/MaxAreaWidth/Height` methods into two methods (I did this for my own layout) with the same semantics. It looks something like this: default double computeSpan(SizeQuery query, Measurable child, double baselineComplement, Insets margin, double extent, boolean fillExtent) { boolean usesBaseline = baselineComplement != -1 && orientation() == Orientation.VERTICAL; if(usesBaseline) { double baseline = child.getBaselineOffset(); if(baseline != BASELINE_OFFSET_SAME_AS_HEIGHT) { return baseline + baselineComplement; } } double dependentExtent = -1; if(extent != -1 && child.getContentBias() == cross().orientation()) { // span depends on cross span double areaExtent = baselineComplement != -1 && orientation() == Orientation.HORIZONTAL && child.getBaselineOffset() == BASELINE_OFFSET_SAME_AS_HEIGHT ? extent - baselineComplement : extent; dependentExtent = cross().computeDependentExtent(child, margin, areaExtent, fillExtent); } return margin(margin) + snapSize(query.compute(this, child, dependentExtent)) + (usesBaseline ? baselineComplement : 0); } default double computeDependentExtent(Measurable child, Insets margin, double areaExtent, boolean fillExtent) { double contentExtent = areaExtent - margin(margin); return fillExtent ? snapSize(boundedSize(min(child, -1), contentExtent, max(child, -1))) : snapSize(boundedSize(min(child, -1), pref(child, -1), Math.min(max(child, -1), contentExtent))); } Basically a single function (`computeSpans`) that does what the `computeChild*` methods do. I'm not entirely sure it will fit well in JavaFX or if it is worth it. But for me the above is alot easier to maintain (the code is equivalent even though it has a bit of a different shape). ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2241#discussion_r3751443565
