On Wed, 29 Mar 2023 17:31:42 GMT, Michael Strauß <mstra...@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 It could perhaps be a reasonable step, although most of the effort would become wasted if we also move #1111 forward. I'd expect a good JUnit test though to prove that it works correctly. It's also possible that the integration of #1118 already solved some of the problems (but I'm sure it did not solve all of them), and so this PR and #1111 may have become less important. ------------- PR Comment: https://git.openjdk.org/jfx/pull/445#issuecomment-1616885353
