RFR: 8278021: Fix warnings in macOS glass native code and treat warnings as errors [v5]
Kevin Rushforth
kcr at openjdk.org
Tue Feb 13 00:37:04 UTC 2024
On Mon, 5 Feb 2024 16:46:40 GMT, Martin Fox <mfox at openjdk.org> wrote:
>> Turning on warnings-as-errors for the macOS glass native code. Deprecated declarations are excluded and still appear as warnings.
>>
>> In the code that tries to locate the application's dock icon there were three instances where `NO` was being passed into a method that required a pointer to a `BOOL`, not a `BOOL`. I suspect the intent was to check that the path pointed to an existing file but not a directory. Since JavaFX has gone this long without screening out directories correctly I decided not to fix that behavior except at the very end.
>>
>> The only other changes of note are sending some NSNotification objects to delegate API's that require them even though we know they're ignored on the other side. It was the easiest way to get rid of the warning.
>
> Martin Fox has updated the pull request incrementally with two additional commits since the last revision:
>
> - Work-around for using a call not present in the Xcode 13.3 SDK.
> - Need correctly formed NSNotification when running tests
I ran a full set of headful tests on macOS 13 and 14.
I also ran some local tests and verified the modified code.
All looks good.
modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m line 655:
> 653: // try again using Java generic icon (this icon might go away eventually ?)
> 654: iconPath = [NSString stringWithFormat:@"%s", "/System/Library/Frameworks/JavaVM.framework/Resources/GenericApp.icns"];
> 655: }
I presume you removed this because it is obsolete code? I do note that the file in question is no longer there, so this seems fine.
-------------
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/687#pullrequestreview-1876247827
PR Review Comment: https://git.openjdk.org/jfx/pull/687#discussion_r1486754653
More information about the openjfx-dev
mailing list