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

[Prasanta Sadhukhan]

Hi Krishna,

But in other area of the code, like

778 if (newWidth > 0) { 779 newState |= WIDTH_FLAG; 780 }

we do check for width/height > 0 before updating the state, so I would 
expect to maintain consistency, we should do the same for
797 newState |= (WIDTH_FLAG | HEIGHT_FLAG);Regards Prasanta
On 06-Mar-19 3:25 PM, Krishna Addepalli wrote:
> 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 
>> <mailto: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>  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>;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/~kaddepalli/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
>>>>>>>>>>> Webrev:http://cr.openjdk.java.net/~kaddepalli/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/372e33fb/attachment-0001.html>