Hi Prasanta,
Ok, lets understand this in terms of the possible cases:
When newWidth > 0 || newHeight > 0 – then we should update the state
anyway. This covers 3 cases in which we need update the state.
The only other case is when newWidth < 0 && newHeight < 0 – as in the
dimensions are not specified in the html. In this case, we need to
fallback to image dimensions, which again will warrant a state update.
So, keeping this in view, I did not write another condition to check
for the width /height change to update the state.
Hope this clarifies the logic.
Thanks,
Krishna
*From:*Prasanta Sadhukhan
*Sent:* Wednesday, March 6, 2019 4:01 PM
*To:* Krishna Addepalli <krishna.addepa...@oracle.com>
*Cc:* Sergey Bylokhov <sergey.bylok...@oracle.com>;
swing-dev@openjdk.java.net
*Subject:* Re: <Swing Dev> [13] RFR: JDK-8218674 - HTML Tooltip with
"img=src" on component doesn't show
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.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@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.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@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.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@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.bylok...@oracle.com>
<mailto:sergey.bylok...@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.addepa...@oracle.com>
<mailto:krishna.addepa...@oracle.com>;swing-dev@openjdk.java.net
<mailto:swing-dev@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
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.
