<Swing Dev> [13] RFR: JDK-8218674 - HTML Tooltip with "img=src" on component doesn't show
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Wed Mar 6 16:51:16 UTC 2019
ok. Thanks for the clarification.
Regards
Prasanta
On 06-Mar-19 5:08 PM, Krishna Addepalli wrote:
>
> 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.addepalli at oracle.com>
> *Cc:* Sergey Bylokhov <sergey.bylokhov 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,
>
> 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>
> <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
>
> 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/0f6b9b63/attachment-0001.html>
More information about the swing-dev
mailing list