<Swing Dev> [13] RFR: JDK-8218674 - HTML Tooltip with "img=src" on component doesn't show

[Krishna Addepalli]

Hi Prasanta,

Handling 0-sized images was not done in the earlier fix as well. And this fix simply makes sure that the earlier fix works exactly the same when the image is loaded synchronously/asynchronously.
At least from the looks of it, I do not see any harm in updating the image size, since it will only load the appropriate image, and update the ImageView object state.

Thanks,
Krishna

> On 05-Mar-2019, at 8:54 AM, Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com> wrote:
> 
> Hi Krishna,
> 
> On 04-Mar-19 5:49 PM, Krishna Addepalli wrote:
>> Hi Prasanta,
>> 
>> Yes, you are right. But, does it make a difference if we still update the values even if they are same?
> I am not pointing to 
> d.width = newWidth;
> 
> My concern is newState |= (WIDTH_FLAG | HEIGHT_FLAG);
>  where you update the state even if width/height can be 0. I am not sure about possible repurcussions? I do not see any test with image of width/height=0 so thought that you probably do not check that path.
> 
> Regards
> Prasanta
>> 
>> Thanks,
>> Krishna
>> 
>>> On 04-Mar-2019, at 5:12 PM, Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>> 
>>> 
>>> 
>>> On 04-Mar-19 4:53 PM, Krishna Addepalli wrote:
>>>> Hi Prasanta,
>>>> 
>>>> Thanks for the review. Here is the updated webrev: http://cr.openjdk.java.net/~kaddepalli/8218674/webrev01/ <http://cr.openjdk.java.net/%7Ekaddepalli/8218674/webrev01/>
>>>> 
>>>> The check (newWidth>0) && (newHeight > 0) is done in adjustWidthHeight function.
>>>> 
>>> if (specifiedWidth != -1 && specifiedHeight != -1) {
>>>  915             newWidth = specifiedWidth;
>>>  916             newHeight = specifiedHeight;
>>>  917         }
>>> ...
>>>  931         d.width = newWidth;
>>>  932         d.height = newHeight;
>>> 
>>> If specifiedWidth/Height is 0, then I do not see any check, it is just assigning to d.width/height, no? In that case, can we need to change the newstate without checking?
>>> 
>>> Regards
>>> Prasanta
>>>> Thanks,
>>>> Krishna
>>>> 
>>>>> On 04-Mar-2019, at 1:29 PM, Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com <mailto:prasanta.sadhukhan at oracle.com>> wrote:
>>>>> 
>>>>> Hi Krishna,
>>>>> You can reuse existing method  getLoadsSynchronously() instead of checking for (state & SYNC_LOAD_FLAG) != 0)
>>>>> 
>>>>> BTW, do we not have to check if (newWidth > 0) & newHeight >0 before changing the newstate @l797?
>>>>> 
>>>>> The copyright year in testcase should be changed to 2019.
>>>>> 
>>>>> Regards
>>>>> Prasanta
>>>>> On 01-Mar-19 12:10 PM, Krishna Addepalli wrote:
>>>>>> Thanks for the review Sergey.
>>>>>> Can I have one more review? Prasanta maybe?
>>>>>> 
>>>>>> Thanks,
>>>>>> Krishna
>>>>>> 
>>>>>>> On 01-Mar-2019, at 3:40 AM, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> <mailto:Sergey.Bylokhov at oracle.com> wrote:
>>>>>>> 
>>>>>>> Looks fine.
>>>>>>> 
>>>>>>> On 21/02/2019 08:44, Sergey Bylokhov wrote:
>>>>>>>> On 20/02/2019 22:21, Krishna Addepalli wrote:
>>>>>>>>> Hi Sergey,
>>>>>>>>> 
>>>>>>>>> I have fixed the issue. Could you check now?
>>>>>>>> Yes, it works now, I look to the fix.
>>>>>>>>> Thanks,
>>>>>>>>> Krishna
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Sergey Bylokhov
>>>>>>>>> Sent: Thursday, February 21, 2019 3:54 AM
>>>>>>>>> To: Krishna Addepalli <krishna.addepalli at oracle.com> <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>>>>>>>>> Subject: Re: <Swing Dev> [13] RFR: JDK-8218674 - HTML Tooltip with "img=src" on component doesn't show
>>>>>>>>> 
>>>>>>>>> Hi, Krishna.
>>>>>>>>> 
>>>>>>>>> Some links have wrong file permissions, "403 - Forbidden":
>>>>>>>>> http://cr.openjdk.java.net/~kaddepalli/8218674/webrev00/raw_files/new/test/jdk/javax/swing/text/html/8218674/circle.png <http://cr.openjdk.java.net/%7Ekaddepalli/8218674/webrev00/raw_files/new/test/jdk/javax/swing/text/html/8218674/circle.png>
>>>>>>>>> http://cr.openjdk.java.net/~kaddepalli/8218674/webrev00/raw_files/new/test/jdk/javax/swing/text/html/8218674/TooltipImageTest.java <http://cr.openjdk.java.net/%7Ekaddepalli/8218674/webrev00/raw_files/new/test/jdk/javax/swing/text/html/8218674/TooltipImageTest.java>
>>>>>>>>> 
>>>>>>>>> On 20/02/2019 03:57, Krishna Addepalli wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>> 
>>>>>>>>>> Please review a fix for the bug JDK-8218674: https://bugs.openjdk.java.net/browse/JDK-8218674 <https://bugs.openjdk.java.net/browse/JDK-8218674>
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~kaddepalli/8218674/webrev00/ <http://cr.openjdk.java.net/%7Ekaddepalli/8218674/webrev00/>
>>>>>>>>>> 
>>>>>>>>>> This is a regression introduced due to fix for JDK-8208638. The default behaviour for ImageView is to load an image asynchronously. Hence, it uses the ImageHandler::imageUpdate to get the updates to the image being loaded. That will set the width and height of the image view. ImageView::updateImageSize does not alter the width and height in this case. When a JToolTip is created and html text set as tooltip, internally, the image is requested to be loaded synchronously, and in this case, ImageView::updateImageSize is the only way to calculate the image size. Since the width and height were not specified in the tooltip, the image was not being drawn.
>>>>>>>>>> The fix is to check if the image is requested to be loaded synchronously, and if so, then do the same calculation as for the fix for JDK-8208638, which will provide valid image width and height, additionally also taking care of the scaling issues fixed for JDK-8208638.
>>>>>>>>>> I have tested the fix on Windows, Linux(Ubuntu) and Mac, and found that it is working. I have also run all the jtreg tests under the test/jdk/javax/swing/text/html, and found no new failures.
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Krishna
>>>>>>> -- 
>>>>>>> Best regards, Sergey.
>>>>> 
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20190306/92f310fa/attachment-0001.html>