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

[Andy Goryachev]

On Fri, 28 Feb 2025 22:09:19 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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.

I think the main concern with the reformatting/tangentially related changes coming from the maintainers is the general pain associated with conflicts created when merging and backporting.

In this case, I think we might be ok, specifically in the tests.  I would rather see the cleaner and more maintainable code, even at the small additional expense.  Up to you.

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

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