On Tue, 26 Jul 2022 09:15:36 GMT, Paul <d...@openjdk.org> wrote: >> I hit on JDK-8181084 while making some changes to Scene Builder, so I >> decided to investigate. >> >> Please note: I have not done any Objective-C or MacOS development before. So >> I would really like some feedback from someone else who knows this stuff >> better. >> >> Anyway, after some googling, I discovered that MacOS uses points values for >> measurements and not pixels, so the actual fix for this issue was this block >> in `GlassMenu.m`:- >> >> >> if ((sx > 1) && (sy > 1) && (width > 1) && (height > 1)) { >> NSSize imgSize = {width / sx, height / sy}; >> [image setSize: imgSize]; >> } >> >> >> The rest of the changes were needed in order to get the `scaleX` and >> `scaleY` values down from `PixelUtils.java` into `GlassMenu.m`. >> >> Before this fix:- >> >> <img width="239" alt="Screenshot 2022-02-26 at 22 16 30" >> src="https://user-images.githubusercontent.com/437990/155880981-92163087-696b-4442-b047-845c276deb27.png"> >> >> After this fix:- >> >> <img width="218" alt="Screenshot 2022-02-26 at 22 18 17" >> src="https://user-images.githubusercontent.com/437990/155880985-6bff0a06-9aee-4db2-b696-64730b0a6feb.png"> > > Paul has updated the pull request incrementally with one additional commit > since the last revision: > > A few more minor code cleanups
The fix looks good with a couple comments inline. I tested it on all platforms (since there is a small change in shared code), and it all looks good. modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 272: > 270: && (jPixelsHeightField > 0) > 271: && (jPixelsScaleXField > 0) > 272: && (jPixelsScaleYField > 0) Since this is a `jField`, it should be tested for non-zero rather than `> 0`, so the better test would be: if (jPixelsWidthField && jPixelsHeightField && jPixelsScaleXField && jPixelsScaleYField) The code should never get here, though, if those fields are not initialized in `initIDs`, so you could remove the check entirely, or else move it to the top of the method (before the test for pixel != null) as a sanity check, and return if any are null. I'll leave that part of the change up to you. modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 279: > 277: jfloat scaley = (*env)->GetFloatField(env, pixels, > jPixelsScaleYField); > 278: > 279: if ((scalex > 1) && (scaley > 1) && (width > 1) && > (height > 1)) { The check for `width` or `height` should be `> 0`, since a width or height of 1 is valid (perhaps not a likely value for an icon, but I see no reason to exclude it). Also, in theory, `scalex` and `scaley` could be different such that one of them is `1` and the other is `> 1`, in which case you would still want to apply the scale. That might not be worth worrying about here (I doubt it can ever happen on macOS). ------------- PR: https://git.openjdk.org/jfx/pull/743