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

[Kevin Rushforth]

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.

PR  #1111 is likely to be the eventual solution for the general problem of handling layout when snap-to-pixel is set (as it is by default). So the question I have, primarily for @mstr2 and @hjohn, is: Is this PR a reasonable step along the path to where we want to go? Or will it need to be reworked or reverted if and when we get to PR #1111 ?

If it's the former, it might make sense to revive this PR and take it forward. Otherwise, it should probably be closed in favor of continuing work on PR #1111

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

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