PS. I'll be pushing the FX part of the fix today. So we should consider the current interface frozen for now.
Yes, please! -- Kevin Anthony Petrov wrote:
On 5/23/2014 3:12 PM, Anton V. Tarasov wrote:On 23.05.2014 14:47, Anthony Petrov wrote:1. The host bounds are not related to the /content/. Hence, adding this method to the LightweightContent interface would look inconsistent from API perspective.It's not strictly about content (the name of the interface is not good enough). 32 * The interface by means of which the {@link JLightweightFrame} class 33 * communicates to its client application.Right. We might want to file a follow-up issue targeted for JDK/FX 9 and rename the interface/rearrange some APIs. However, personally, I don't feel this is strictly necessary at this time.PS. I'll be pushing the FX part of the fix today. So we should consider the current interface frozen for now.-- best regards, Anthony2. Given the requirement to keep backward compatibility, the default implementation of the method would return 'null', so the calling code would have to check the return value and fall back to calling LF.getBounds() manually. Currently this logic is encapsulated in the LightweightFrame class itself, which looks reasonable to me. 3. SwingNode already calls other APIs on LF, such as notifyDisplayChanged() (and again, this particular notification is unrelated to the /content/ itself.) So adding the setHostBounds() to LF looks consistent from this perspective, too. 4. The current implementation of the getHostBounds() method simply returns a new rectangles using cached values. If we implement your suggestion, then every call to CPLWW.getGraphicsDevice() would involve an additional call to the SwingNode code, which may impact the performance slightly. 5. I was almost ready to push the FX part of the fix today, and let's admit it, this fix is very well overdue. I'd prefer if we don't modify the interface anymore.Ok, this is about a matter of taste. The 5th point sounds strong enough for me, so I'm fine with the current version. Thanks! Anton.-- best regards, Anthony On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:Hi Sergey, Thanks for the update. I'm fine with the fix, except one thing. (I'm sorry that I didn't say that earlier).My concern is that we have the LightweightContent iface which is used tocommunicate to the client app. And so the method LightweightFrame.getHostBounds()is better to be a method of that iface which the client (SwingNode) willimplement on its side. In that case we won't need the LightweightFrame.setHostBounds() method. This would look consistent from my perspective. Thanks, Anton. On 22.05.2014 22:05, Sergey Bylokhov wrote:On 5/22/14 5:58 PM, Anton V. Tarasov wrote:On 22.05.2014 15:36, Sergey Bylokhov wrote:On 5/22/14 11:47 AM, Anton V. Tarasov wrote:Hi Sergey, On 22.05.2014 1:44, Sergey Bylokhov wrote:On 5/21/14 10:13 PM, Anthony Petrov wrote:Hi Sergey, The original fix provides some updates and clarifications to thejavadoc for the LightweightContent.imageBufferReset() method, butthey are missing from your fix. Is this intentional?Nope. I just missed this update. I looked at this method closelyand got a question: do we need this scale parameter? Why we cannotuse w,h + scanstride here an skip all clarification about logical coordinates?Originally, Jim suggested to generalize the API:<<Rather than imply any parameters, I think specifying a very exactset of parameters gives the most flexibility. Even if the relationships you characterize above are true, xywh,scan or off,wh,scan both provide the flexibility to supply the data inthose formats without the client having to guess dimensions or scan size. Any API that specifies an array containing data should alwaysprovide the flexibility of specifying an offset (and x,y is a wayof specifying an offset for rectangular data and using a nio Buffer can implicitly imply an offset based on its position) and when thatdata is a rectangle of data then it should also supply independent w,h and scan strides. If the offset is always 0, and if thescanstride is always w in the implementation's that choose the datastorage then it may seem like overkill, but it provides the flexibility of switching to a more sophisticated buffer re-usestrategy later without having to track down every client and updatethem... >> and so we provide all the coordinates.I understand why we need x/y/w/h/scanstride but why we need scale, because our buffer is pixel based anyway? In this case we have to convert w/h/x/y/scanstride from logical to pixels and back.The reasoning for that if the following. On the client side (SwingNode), during the rendering of the image, there's a need to have logical bounds of the image. So, this would require conversion (devision) for what the client would need to know the scale factor for what it would need to ask for it, separately. This would bringanother code path & dependencies (for instance, b/w SwingNode and itsprism peer). Currently, there's only one parameter of a method for that purpose.Ok. Here is an updated version: http://cr.openjdk.java.net/~serb/8029455/webrev.02Thanks, Anton.Thanks, Anton.BTW, I've applied your fix and tested it with the latest versionof FX changes, and everything works smoothly on my Mac, includingthe display change listener. -- best regards, Anthony On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:Hello, Everybody. Please review an updated version of this fix. This is a minimum possible fix. changes in shared code of jdk are minimized and can be enhanced in the future. The fix is covering hdpi support in SwingNode on osx + system look and feel(Aqua). http://cr.openjdk.java.net/~serb/8029455/webrev.01 Notes: - This fix depends from two other fixes: JDK- 8041129 and JDK-8041644. Both are under review on 2d alias. On 5/13/14 9:29 PM, Anthony Petrov wrote:Hi Jim, Sergey, and Anton, I'd like to revive this old thread and finally push this fix, which has been reviewed and approved on this mailing list back in February. The only additional change that I want to introduce, is the addition of default implementations for the LightweightContent.imageBufferReset() methods. This allows clients of the API (namely, JavaFX) to run with both the old and the new JDK w/o any changes or side-effects. Here's the updated webrev:http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/It literally only adds the default methods and doesn't make anyother changes to the rest of the already reviewed code. I've tested this on both hiDPI and loDPI displays, with both old and hiDPI-aware JavaFX builds, and it works fine in all the cases. The current plan is to push this fix to JDK 9, and then back-port the changes to 8u20. -- best regards, Anthony On 2/21/2014 2:47 AM, Jim Graham wrote:Yes, approved. ...jim On 2/17/14 6:09 AM, Anton V. Tarasov wrote:Jim, so this is ready for a push then. Thanks! Anton. On 15.02.2014 5:01, Jim Graham wrote:I don't need to see an update for that. I didn't read the entire webrev, but I looked at this one piece of code and if that was the only thing changed then I think that dealt with the outstanding issues... ...jim On 2/13/14 11:12 PM, Anton V. Tarasov wrote:On 14.02.2014 2:52, Jim Graham wrote:On 2/13/14 5:03 AM, Anton V. Tarasov wrote:Hi Jim, Please, look at the update: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5Here I'm correcting the rect after the transform in SG2D:2123 // In case of negative scale transform, reflect the rect coords. 2124 if (w < 0) { 2125 w *= -1; 2126 x -= w; 2127 } 2128 if (h < 0) { 2129 h *= -1; 2130 y -= h; 2131 } The blit direction (dx, dy) remains transformed. Is this the right behavior from your perspective?Yes, that looks good. I wonder if the "w *= -1" results in a multiply byte code whereas "w = -w" would avoid the multiply? ...jimJim, Yes, this indeed results in different byte code instructions (imult &ineg). Just for curiosity I did some measuring which showednegatioation worked 10% faster :) Well, I'll fix it but let me please not send an update... Thanks! Anton.
