RFR: 8230833: LabeledSkinBase computes wrong height with ContentDisplay.GRAPHIC_ONLY [v5]

Karthik P K kpk at openjdk.org
Thu Feb 9 13:45:00 UTC 2023


On Wed, 8 Feb 2023 20:03:05 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Addressing review comments
>
> For your convenience, I've added a Label page to the monkey tester
> https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/monkey/MonkeyTesterApp.java
> 
> (sorry, it is still in a separate repository.  if you use Eclipse, just import the project into your workspace)

@andy-goryachev-oracle thanks for the code snippet and adding the Label page to the Monkey Tester.

In case of width calculation, the optimizations done in the code snippet is better since we avoid some computation. I implemented similar logic to `computePrefHeight()` method. 
With this optimization, for a case like empty text and empty graphic, the computed value of height will be 0.
This creates issue with some of the unit tests in `LabelSkinTest` because the expectation is that `computePrefHeight()` method returns single line string height for these cases. This is observed in unit test cases where we calculate preferred height and max height (max height also calls `computePrefHeight()` method).
Behaviorally there is no difference because a different method calculates the min height which returns the minimum height of a single line for the cases where text is empty and graphic is also null. But looking at the expected behavior of `computePrefHeight()` method from unit test cases, it looks like single line string height needs to be returned from this method in which case we will not be able to use this optimized `if-else` block since we have to calculate textHeight regardless of any conditions.
So if we have to optimize the `computePrefHeight()` method, the expected behavior also changes in these conditions.

Please let me know your thoughts on this.

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

PR: https://git.openjdk.org/jfx/pull/996


More information about the openjfx-dev mailing list