RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v7]

[John Hendrikx]

On Wed, 29 Mar 2023 17:31:42 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>>> > @andy-goryachev-oracle raises a good point about perhaps wanting to solve this in a similar way for the various places where we need to do some sort of iterative adjustment (such as here and in the eventual solution for JDK-8299753). It seems also worth looking at what @Maran23 proposes for AnchorPane to fix JDK-8295078 in PR #910 (for which the review still needs to be completed).
>>> 
>>> Yes, at this point we should probably discuss what would be the best solution to fix the snapping issues once and for all. We might even want to look into other frameworks like Swing. Maybe there are people internally at Oracle who can give valuable input here too?
>> 
>> I think extracting the common code out of HBox and VBox would be good, as they're basically doing the same thing in a different axis.  The code could then be unit tested directly (ie. given a set of min/max/pref sizes, a target size and a "snapper", calculate optimal sizes).  Something like this:
>> 
>>      double[] distribute(double space, double[] minWidths, double[] maxWidths, Snapper snapper);
>> 
>> The minWidths/maxWidths would be min + pref or pref + max depending on the situation, but this function wouldn't care.  The `Snapper` here would be aware of the scale, and would be a dummy implementation that does nothing when snapping is disabled.
>> 
>> This purposely does not handle spacing between children, as you can calculate that before hand and adjust `space` to the actual space available for children, simplifying the code.
>> 
>> Disclaimer: I used to create my own layout manager (see: [smartlayout](https://github.com/hjohn/hs.ui.smartlayout/blob/master/src/main/java/hs/smartlayout/LayoutRequirements.java)) that took into account min/max and weight of each child, and even groups of children (a restriction over two or more children at once) -- weight would affect resizing, allowing you to make layouts where children took space according to some ratio (1:2:3 for example).  This functionality I called a `SpaceDistributor` which was purposely given a neutral name as it could be used for both horizontal or vertical calculations.  When including weight, the calculations could get so complex that I had multiple implementations and an exhaustive search implementation (which was used by unit tests to verify the optimized implementations). This was before scaling became an issue though, so this code did not take snapping values to pixels into account.
>
> Thank you very much @hjohn for your extensive review. Before addressing your comments, I'd like to get some clarification (ideally also from @kevinrushforth), because I am a bit confused by the discussion so far.
> 
> This PR is a very narrow patch that fixes an obvious bug in `HBox` and `VBox`. It's narrow in that it doesn't change the present algorithm, it merely corrects some of its flaws. There is a little bit of refactoring, but that's only done where it helps readers to better understand the algorithm.
> 
> However, the discussion seems to center around the idea of large-scale refactoring, even across multiple components, including  open tickets like [8299753](https://bugs.openjdk.org/browse/JDK-8299753). That's not something I can do as part of this PR, and if large-scale refactoring is the way we choose to go forward, I'd rather not spend more of my time on this particular PR. There needs to be a realistic chance that this PR will be accepted for integration basically as it is.
> 
> If we want to expand the scope of this work, this PR should be closed and the discussion should be continued on the mailing list.

@mstr2 I noticed in your test that you said that when `prefWidth` is set, the container must use exactly that size, but this is not entirely true. It just so happens that `76.0` is a pixel boundary for all the render scales you tested.  If you use a different width or use a different render scale (like `1.33333333`) then it won't be a pixel boundary and the width will not be exactly what you specified for preferred width.  With pref width `76.0` and render scale `1.33333333` the final width will be `76.5` for example.

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

PR Comment: https://git.openjdk.org/jfx/pull/445#issuecomment-1523144658