On Wed, 29 Mar 2023 10:20:12 GMT, John Hendrikx <jhendr...@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? > >> > @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. Yes, I think it's an improvement in filling the boxes properly (I asked for some clarification as I found the code hard to read, but that's probably how you found it). My only worry is that it fixes this specific problem, but may be introducing a new issue (I think snapping values multiple times can introduce artifacts in the child sizes, even though they will all align correctly, I think they might be off a bit). If you can confirm this isn't the case, or if it is, fix those last problems then I think it is a positive change. I have noticed similar problems on my system where I don't use 100 or 200% scaling, and it would be good to see them fixed. > 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. Those were possible future directions I suppose, not necessarily as part of this PR. I dislike the code duplication, but it was that way already. ------------- PR Comment: https://git.openjdk.org/jfx/pull/445#issuecomment-1489314876
