Re: RFR: 8181084: JavaFX show big icons in system menu on macOS with Retina display [v5]

[Jose Pereda]

On Sat, 23 Jul 2022 07:51:05 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 with a new target base due to a merge or a 
> rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains seven additional commits since 
> the last revision:
> 
>  - Merge branch 'master' into JDK-8181084
>  - Merge branch 'master' into JDK-8181084
>  - Updated variable names to be a bit more consistent
>  - Merge branch 'master' into JDK-8181084
>  - Merge branch 'master' into JDK-8181084
>  - Removing trailing whitespace.
>  - Correctly scales hi-res icons in MacOS system menu items

Looks good (and tested on Mac). I have some comments.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkPixels.java line 
34:

> 32: final class GtkPixels extends Pixels {
> 33: 
> 34:     public GtkPixels(int width, int height, ByteBuffer data, float 
> scalex, float scaley) {

minor: move it after the existing `public GtkPixels(int width, int height, 
ByteBuffer data)`

modules/javafx.graphics/src/main/java/com/sun/glass/ui/ios/IosApplication.java 
line 128:

> 126:     }
> 127: 
> 128: 

minor: remove extra line

modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacPixels.java line 
52:

> 50:     }
> 51: 
> 52:     protected MacPixels(int width, int height, ByteBuffer data, float 
> scalex, float scaley) {

minor, move it after `MacPixels(int width, int height, ByteBuffer data)`

modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MonocleApplication.java
 line 193:

> 191:     public Pixels createPixels(int width, int height, ByteBuffer data,
> 192:                                float scalex, float scaley)
> 193:     {

minor: move curly brace right after the parenthesis of the previous line.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinPixels.java line 
53:

> 51:     }
> 52: 
> 53:     protected WinPixels(int width, int height, ByteBuffer data, float 
> scalex, float scaley) {

minor: move it right after `WinPixels(int width, int height, ByteBuffer data)`

modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 265:

> 263:         if (image != NULL)
> 264:         {
> 265:             jclass pixelsClass = (*env)->FindClass(env, 
> "com/sun/glass/ui/mac/MacPixels");

Use `[GlassHelper ClassForName:"com.sun.glass.ui.mac.MacPixels" withEnv:env];` 
instead, 
and then check `pixelsClass` is not null.

modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 267:

> 265:             jclass pixelsClass = (*env)->FindClass(env, 
> "com/sun/glass/ui/mac/MacPixels");
> 266: 
> 267:             jfieldID jPixelsWidthField = (*env)->GetFieldID(env, 
> pixelsClass, "width", "I");

After each JNI call you need to check for exceptions like:

jfieldID jPixelsWidthField = (*env)->GetFieldID...
if ((*env)->ExceptionCheck(env)) return;

modules/javafx.graphics/src/main/native-glass/mac/GlassMenu.m line 270:

> 268:             jfieldID jPixelsHeightField = (*env)->GetFieldID(env, 
> pixelsClass, "height", "I");
> 269:             jfieldID jPixelsScaleXField = (*env)->GetFieldID(env, 
> pixelsClass, "scalex", "F");
> 270:             jfieldID jPixelsScaleYField = (*env)->GetFieldID(env, 
> pixelsClass, "scaley", "F");

As Kevin already commented, these four fields can be cached and set in 
`initIDs`.

-------------

PR: https://git.openjdk.org/jfx/pull/743