<AWT Dev> [9] Review request for 8043869 [macosx] java -splash does not honor @2x hi dpi notation for retina support

[Anthony Petrov]

Hi Alexander,

The fix looks good to me overall. One question though, regarding the 
file name-related logic in splashscreen_sys.m:

>  136         NSString *fileName = [NSString stringWithUTF8String: file];
>  137         NSUInteger length = [fileName length];
>  138         NSRange range = [fileName rangeOfString: @"."
>  139                                         options:NSBackwardsSearch];
>  140         int index = range.location;
>  141
>  142         if (index != NSNotFound && index != 0 && index != length-1) {
>  143             NSString *fileName2x = [fileName substringToIndex: index];
>  144             fileName2x = [fileName2x stringByAppendingString: @"@2x"];
>  145             fileName2x = [fileName2x stringByAppendingString:
>  146                           [fileName substringFromIndex: index]];
>  147
>  148             if (jar || [[NSFileManager defaultManager]
>  149                           fileExistsAtPath: fileName2x]){

Does this code work well if the image file name doesn't have an 
extension? There was a related issue in FX and it was fixed with the 
following changeset:
http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/d202ef8951c9
Please check if your code is ready to handle similar situations.

--
best regards,
Anthony

On 6/18/2014 5:03 PM, Alexander Scherbatiy wrote:
>
>   Hello,
>
>   Could you review the updated fix:
>      http://cr.openjdk.java.net/~alexsch/8043869/webrev.02
>
>    - formatting and CR-LF line endings are fixed
>    - the condition if (1 < screenScaleFactor) is rewritten in
> splashscreen_sys.m file
>    - file_name == NULL chec is added in the java.c
>    - [NSScreen mainScreen] is changed to SplashNSScreen() in the
> SplashGetScaledImageName() from splashscreen_sys.m file
>
> Thanks,
> Alexandr.
>
> On 6/17/2014 8:21 PM, Kumar Srinivasan wrote:
>> Hello,
>>
>> As Anthony already commented, there are some formatting issues
>> throughout, please
>> retain the existing convention and formatting.
>>
>> src/share/bin/java.c
>> at 1822, if you add
>> if (file_name == NULL){
>>     return;
>> }
>> and removing the else, at the bottom we can reduce the indent of the
>> exist if block.
>>
>> make/mapfiles/libsplashscreen/mapfile-vers
>> +1, good to note this, always trips people up.
>>
>>
>> Otherwise looks good.
>>
>> Kumar
>>
>>
>>
>>
>> On 6/17/2014 7:45 AM, Anthony Petrov wrote:
>>> Hi Alexander,
>>>
>>> [ I'm also adding Neil who's taking over the launcher code ]
>>>
>>> 1. There's a few code formatting issues that should be fixed. For
>>> instance:
>>> src/share/bin/java.c
>>>> 1846             if(scaled_splash_name){
>>>> 1853         if(scaled_splash_name){
>>>
>>> src/windows/native/sun/awt/splashscreen/splashscreen_sys.c
>>>>  571     *scaleFactor=1;
>>>
>>> In all the above lines required spaces are missing. I believe there's
>>> a number of other occurrences of the same issue in your patch. Please
>>> check it carefully and fix the formatting on all lines.
>>>
>>> 2. Webrev shows 236 changed lines for java_awt_SplashScreen.c. I
>>> suspect you've changed the EOL characters because you've edited your
>>> code on MS Windows. Please configure your editor to use LF characters
>>> for line ends and revert the unnecessary changes (note that jcheck
>>> won't let you push CR-LF line endings anyway).
>>>
>>> 3. splashscreen_sys.m
>>>>  135     if (1 < screenScaleFactor) {
>>>
>>> For consistency and clarity, I'd suggest to rewrite the condition as
>>>
>>>     if (screenScaleFactor > 1) {
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 6/17/2014 6:20 PM, Petr Pchelko wrote:
>>>> Hello, Alexander.
>>>>
>>>>>> 2. In java.c:1859 DoSplashSetFileJarName sets the name of the file
>>>>>> which was used for a splash. It can be
>>>>>> retrieved via public API SplashScreen.getImageURL. Is it correct
>>>>>> to always set the file_name disregards the real
>>>>>> name we've used?
>>>>>
>>>>>     Yes. The original splash screen image name and size are
>>>>> provided for the developer even the high resolution splash screen
>>>>> is shown.
>>>> Thank you for the clarification.
>>>>
>>>> The updated version looks fine to me.
>>>>
>>>> With best regards. Petr.
>>>>
>>>> On 17 июня 2014 г., at 18:16, Alexander Scherbatiy
>>>> <alexandr.scherbatiy at oracle.com> wrote:
>>>>
>>>>>
>>>>> Hello,
>>>>>
>>>>> Could you review the updated fix:
>>>>>   http://cr.openjdk.java.net/~alexsch/8043869/webrev.01/
>>>>>
>>>>>
>>>>> On 6/17/2014 4:22 PM, Petr Pchelko wrote:
>>>>>> Hello, Alexander.
>>>>>>
>>>>>> CCed Kumar as I believe he's the owner of the launcher code.
>>>>>>
>>>>>> 1. In splashscreen_sys.m you autorelease the fileName. But I do
>>>>>> not see where the autoreleasepool is set up.
>>>>>> Wouldn't it be better to set up an autoreleasepool in this method
>>>>>> explicitly?
>>>>>      NSAutoreleasePool is added.
>>>>>> 2. In java.c:1859 DoSplashSetFileJarName sets the name of the file
>>>>>> which was used for a splash. It can be
>>>>>> retrieved via public API SplashScreen.getImageURL. Is it correct
>>>>>> to always set the file_name disregards the real
>>>>>> name we've used?
>>>>>
>>>>>     Yes. The original splash screen image name and size are
>>>>> provided for the developer even the high resolution splash screen
>>>>> is shown.
>>>>>
>>>>>     Thanks,
>>>>>     Alexandr.
>>>>>> Thank you.
>>>>>> With best regards. Petr.
>>>>>>
>>>>>> On 17 июня 2014 г., at 15:36, Alexander Scherbatiy
>>>>>> <alexandr.scherbatiy at oracle.com> wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the fix:
>>>>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8043869
>>>>>>>   webrev: http://cr.openjdk.java.net/~alexsch/8043869/webrev.00
>>>>>>>
>>>>>>>   The scaleFactor field is added to the Splash struct.
>>>>>>>   The SplahsScreen class scales the graphics and the size
>>>>>>> according to the scale factor.
>>>>>>>   The native system tries to load high resolution splash image on
>>>>>>> HiDPI display.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>
>>>>
>>
>