[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8264591: HBox/VBox child widths pixel-snap to wrong value [v7]
From:       Kevin Rushforth <kcr () openjdk ! org>
Date:       2023-06-29 21:15:11
Message-ID: veOwNY0BuwD-ML21C_-RpHvlnlrNno01Xx8Qqshw-LU=.a07efdaa-d18b-4f98-9e13-c13fc0539068 () github ! com
[Download RAW message or body]

On Wed, 29 Mar 2023 17:31:42 GMT, Michael Strauß <mstrauss@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


[prev in list] [next in list] [prev in thread] [next in thread]