RFR: 8350149: VBox ignores bias of child controls when fillWidth is set to false [v2]

[John Hendrikx]

On Fri, 28 Feb 2025 21:14:54 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> I just copied the style that was being used, but can make any changes desired of course (at the expense of increasing the diff and amount of code to review).
>
> I know it's a general policy not to do unrelated changes or reformatting, but I think in this case 
> a) it's a test and
> b) you already touched it
> so might as well.

I can do this, but realize that the test is large, and there's comments like this in multiple places, also places I didn't touch.  So, I can fix it here locally, resulting in this method standing out from the rest, or I can do this everywhere causing this PR to have a lot of non-changes that reviewers will need to be aware of and ignore.

Normally I would do this by marking a commit as a "refactor" commit, in which functionality changes are prohibited, and so reviewers only need to look for changes that jump out as not just a minor refactor / rename / move.  However, in the JDK repositories this isn't so easily done, and such commits are not retained (it is squashed together with functional commits), and so to keep them separated you must make a new ticket, new PR, and get new buy-in to get it included.

I'm really trying to avoid to make a commit too large to review and drown out the functional changes with cosmetic changes, even though every time I touch code like `CssStyleHelper` it itches to remove redundant checks or give fields/variables more descriptive names.  Even the changes I did in Region I was already questioning if this wouldn't be too much (there is more that I would have liked to change there, but I refrained as I fear this will just make it too hard to review).  I face a similar dilemma with HBox/VBox modifications... do I do the same modifications twice (for the sake of reviews) or do I introduce an `AbstractBox`, remove all duplicate code, and then apply the modifications there, ensuring that it will be almost impossible to review and seeing what I actually changed...

What we really need IMHO is a way to include refactors with functional changes.  In my day job, we do this by having a specific commit order; we have one or a couple of commits that do minor cleanups or refactors -- no functionality is changed in these commits, leading to quick and easy reviews of those changes.  Then the final commit (we force push to keep commits clean and focused during reviews -- the review tooling we use handles this without any problem) contains **only** the functional changes which, thanks to the earlier refactors, are often tiny changes that just change some parameters or add a function or two -- no noise, and reviewers can focus purely on functionality.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1723#discussion_r1976060714