RFR: 8255415: Nested calls to snap methods in Region give different results

[Jose Pereda]

On Mon, 26 Oct 2020 10:03:20 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> @kleopatra Thanks for checking. 
>> 
>> As discussed in the JBS issue, there is a call in `CheckBoxSkin::layoutChildren` that, probably by mistake, calls `snapSizeX` twice, and that gives different results when a different UI scale is used.
>> 
>> This PR applies to the snapSizeXX methods, so it is guaranteed that `snapSizeXX(snapSizeXX(a)) == snapSizeXX(a)`. With the included test, _this_ issue should now be fixed.
>> 
>> However, you are totally right, the CheckBox test from the JBS issue only works fine with this PR either if `Stage::sizeToScene` is called or if the Scene is created with a defined width, so we still have another issue laying around.
>> 
>> Running the CheckBox test on Windows with 1.75% UI scale and without calling `sizeToScene`, produces this log of width and height values: 
>> 
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.0
>> ...
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.0
>> ...
>> // WindowEvent.RESIZE event UI scale 1.75
>> ...
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.14285659790039
>> 
>> Both values should be snapped, as these are generated with snap methods in HBox. However, this is not the case for height, as there are some conversions in between:  This 33.14285659790039 value comes from an int (`View`: height = (int) (33.0 x 1.75) = 58) to float (`TKSceneListener`: h = 33.142857) to double (`Scene` width = 33.14285659790039). However, the proper snapped value should be 33.14285714285714.
>> 
>> However, if we add a call to  `stage.sizeToScene()`, then the log is:
>> 
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.0
>> ...
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.0
>> ...
>> Scene::resizeRootOnSceneSizeChange 256.0, 33.14285659790039
>> ...
>> Scene::resizeRootOnSceneSizeChange 257.1428571428571, 33.14285714285714
>> ...
>> Scene::resizeRootOnSceneSizeChange 257.1428571428571, 33.14285714285714
>> ...
>> both width and height have stable snapped values, and the checkbox layout is correct.
>> 
>> Back to the 256.0 value of width, this value comes from margin (8.0 * 2) + controls width(54.0 * 4)  + spacing (8.0 * 3), but the value 54.0 is not properly computed, as at this point the UI scale is still 1.0. 
>> Once the UI scale is set to 1.75 the checkBox width is 54.285714285714285, and the scene's width is 257.14285714285714.
>> 
>> So the issue can be related to the UI scale value taken into account too late, and how this change doesn't trigger an update of the scene. 
>> 
>> Does it make sense to address this issue with the current PR?
>
> thanks for the additional info - good dig �� 
> 
> As you noted, there are two issues: 
> 
> - the implementation of the scaled snaps to make them stable (against nested calls) BTW, any reason snappedXXInset are not scaled?
> - a timing/sequence issue with applying the scale 
> 
> So might separate them out in different issues, the first with a new one with this PR fixing it. Depends a bit on granularity of issues - personally, would prefer it, though, if only to make snapping issues more prominent.

OK, https://bugs.openjdk.java.net/browse/JDK-8255415 has been filed (and the current PR targets that issue). 
https://bugs.openjdk.java.net/browse/JDK-8199592 will remain open until a fix for using the correct UI scale to perform the preferred size calculations is proposed.

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

PR: https://git.openjdk.java.net/jfx/pull/336