On Wed, 22 Feb 2023 06:25:46 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> Damon Nguyen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove redundant check and reuse var.
>
> src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 343:
>
>> 341: final String text;
>> 342: final View v = (View)c.getClientProperty(BasicHTML.propertyKey);
>> 343: if (v != null && b.getIcon() == null) {
>
> I think you can store `b.getIcon` same as `text` variable and reuse it down
> below too as it is now being called more than once
Good idea. Added
> src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java line 344:
>
>> 342: final View v = (View)c.getClientProperty(BasicHTML.propertyKey);
>> 343: if (v != null && b.getIcon() == null) {
>> 344: // use zero insets for view when HTML contains an image
>
> I guess it should be "**does not** contain an image", right?
This comment is confusing now that it deals with button icon images and HTML
images. I updated the comment
> test/jdk/javax/swing/JButton/HtmlButtonWithTextAndIcon.java line 27:
>
>> 25: /* @test
>> 26: * @bug 8302173
>> 27: * @requires (os.family == "mac")
>
> Is this required? HtmlButtonImageTest is being tested for all platforms even
> though fix was in mac so I think we should make this test also
> platform-agnostic..
I would but different L&F's show HTML content differently. For example, here's
Metal:
Other L&F's are slightly different as well, and this would require the test to
handle each L&F differently. The changes were only for Aqua L&F, and the
original issue fix was also for only Aqua L&F. The original issue was that the
HTML image (not icon) would appear outside the bounds of the button, similar to
the icon here.
So, I made this test only for macOS && Aqua L&F.
-------------
PR: https://git.openjdk.org/jdk/pull/12520
