<AWT Dev> [9] Review request for JDK-8151787 Unify the HiDPI splash screen image naming convention

Mon May 2 09:11:03 UTC 2016

> On May 2, 2016, at 10:53, Rajeev Chamyal <rajeev.chamyal at oracle.com> wrote:
> 
> Hello Hendrik,
> 
> Thanks for the review.

Rajeev,

please understand that I just gave some comments/opinions—I do not have reviewer status. You might want to wait for a *real* review!

> Please review the updated webrev.
> 
> 1) I’d remove the space between OS name and colon—unless you line up the colons to improve readability.
> Removed the colons as suggested.

I guess you mean the spaces.

> 
> 2) The message also seems to imply that the “image.java-scale2x.ext” format is only supported on Linux and and Mac, but not on Windows. Is that the case? (haven’t looked at the impl.)
> 
> “image.java-scale2x.ext” name format is supported on linux and mac only. On mac and linux currently we are supporting scale factor of 2 only.
> On windows scale factor ranging from 1 and 2(inclusive) is supported. So, scale factor can be a floating point value e.g. for a screen with dpi value of 120 the scale factor would be 1.25.

I didn’t follow the discussion, so please don’t see this as anything else but a comment from the sideline. Really? We are unifying the naming and what works on OS X won’t work on Windows? What happened to WORA? Why not support image.java-scale2x.ext on Windows as well? (Please ignore this, if it has been discussed sufficiently.)

Also, I’m not a native English speaker, but I’d spell “non scaled” with a hyphen, i.e. “non-scaled” or perhaps use “unscaled”. But I guess a native speaker should comment on that.

Just my 2c.

-hendrik

> 
> -----Original Message-----
> From: Hendrik Schreiber [mailto:hs at tagtraum.com] 
> Sent: 02 May 2016 13:55
> To: Rajeev Chamyal
> Cc: Ambarish Rapte; Alexander Scherbatiy; awt-dev at openjdk.java.net; Sergey Bylokhov; Avik Niyogi
> Subject: Re: <AWT Dev> [9] Review request for JDK-8151787 Unify the HiDPI splash screen image naming convention
> 
> Hey,
> 
> Just looked at the launcher.properties (out of curiosity). Here’s some feedback.
> 
>>  87 \    -splash:<imagepath>\n\
>>  88 \                  show splash screen with specified image\n\
>>  89 \                  HiDPI scaled image is also supported\n\
>>  90 \                  For a non scaled image file image.ext below are the scaled image names\n\
>>  91 \                  Windows : image.java-scale<dpi-value>.ext e.g.image.java-scale196.ext\n\
>>  92 \                  Linux : image.java-scale2x.ext\n\
>>  93 \                  Mac : image at 2x.ext also supports image.java-scale2x.ext\n\
> 
> I’d remove the space between OS name and colon—unless you line up the colons to improve readability.
> 
> The message also seems to imply that the “image.java-scale2x.ext” format is only supported on Linux and and Mac, but not on Windows. Is that the case? (haven’t looked at the impl.)
> 
> If it's the case, I’d consider it a bug.
> 
> Cheers,
> 
> -hendrik
> 
> 
> 
>> On May 2, 2016, at 10:07, Rajeev Chamyal <rajeev.chamyal at oracle.com> wrote:
>> 
>> Hello All,
>> 
>> 
>> 
>> Please review the updated webrev.
>> 
>> 
>> 
>> http://cr.openjdk.java.net/~rchamyal/8151787/webrev.03/
>> 
>> Added launcher.properties file to webrev.
>> 
>> Updated the command line description for -splash:<imagepath> option.
>> 
>> 
>> 
>> Regards,
>> 
>> Rajeev Chamyal
>> 
>> 
>> 
>> From: Ambarish Rapte
>> Sent: 02 May 2016 12:01
>> To: Alexander Scherbatiy; Rajeev Chamyal; awt-dev at openjdk.java.net; 
>> Sergey Bylokhov
>> Subject: RE: <AWT Dev> [9] Review request for JDK-8151787 Unify the 
>> HiDPI splash screen image naming convention
>> 
>> 
>> 
>> Hi Rajeev,
>> 
>>                The fix looks good to me.
>> 
>> 
>> 
>> Regards,
>> 
>> Ambarish
>> 
>> 
>> 
>> From: Alexander Scherbatiy
>> Sent: Friday, April 29, 2016 5:18 PM
>> To: Rajeev Chamyal; awt-dev at openjdk.java.net; Sergey Bylokhov
>> Subject: Re: <AWT Dev> [9] Review request for JDK-8151787 Unify the 
>> HiDPI splash screen image naming convention
>> 
>> 
>> 
>> 
>> The fix looks good to me.
>> 
>> Thanks,
>> Alexandr.
>> 
>> On 29/04/16 13:49, Rajeev Chamyal wrote:
>> 
>> Hello Alexandr,
>> 
>> 
>> 
>> Please review the updated fix.
>> 
>> http://cr.openjdk.java.net/~rchamyal/8151787/webrev.02/
>> 
>> 
>> 
>> - Will the java-scale2x image be chosen if fileName2x is nil on the line 163?
>> 
>> fileName2x cannot be nil SplashGetScaledImageName is called only if a 
>> not value is passed in filename and other parameters to new method findScaledImageName are constants.
>> 
>> - Could you add the use case where both @2x and java-scale2x are provided and @2x is chosen in the test?
>> 
>> I have updated the test as suggested.
>> 
>> 
>> 
>> Regards,
>> 
>> Rajeev Chamyal
>> 
>> 
>> 
>> 
>> 
>> From: Alexandr Scherbatiy
>> Sent: 28 April 2016 00:20
>> To: Rajeev Chamyal; awt-dev at openjdk.java.net; Sergey Bylokhov
>> Subject: Re: <AWT Dev> [9] Review request for JDK-8151787 Unify the 
>> HiDPI splash screen image naming convention
>> 
>> 
>> 
>> On 4/27/2016 7:40 PM, Rajeev Chamyal wrote:
>> 
>> 
>> Hello Alexandr,
>> 
>> 
>> 
>> Please review the updated fix.
>> 
>> http://cr.openjdk.java.net/~rchamyal/8151787/webrev.01/
>> 
>> 
>> 162         fileName2x = findScaledImageName(fileName, dotIndex, @"@2x");
>> 163         if(fileName2x != nil && ![[NSFileManager defaultManager]
>> 164                                   fileExistsAtPath: fileName2x]) {
>> 165             fileName2x = findScaledImageName(fileName, dotIndex, @".java-scale2x");
>> 166         }
>> 
>> - Will the java-scale2x image be chosen if fileName2x is nil on the line 163?
>> - Could you add the use case where both @2x and java-scale2x are provided and @2x is chosen in the test?
>> 
>> Thanks,
>> Alexandr.
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> Regards,
>> 
>> Rajeev Chamyal
>> 
>> 
>> 
>> From: Alexandr Scherbatiy
>> Sent: 26 April 2016 14:22
>> To: Rajeev Chamyal; awt-dev at openjdk.java.net; Sergey Bylokhov
>> Subject: Re: <AWT Dev> [9] Review request for JDK-8151787 Unify the 
>> HiDPI splash screen image naming convention
>> 
>> 
>> 
>> On 4/26/2016 11:13 AM, Rajeev Chamyal wrote:
>> 
>> 
>> 
>> Hello All,
>> 
>> 
>> 
>> Could you please review the following fix.
>> 
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8151787
>> 
>> Webrev : http://cr.openjdk.java.net/~rchamyal/8151787/webrev.00/
>> 
>> 
>> 
>> This is a small enhancement to support similar HiDPI splash image name convention on all platforms.
>> 
>> 
>> 
>> Currently we have different naming convention for scaled different platforms.
>> 
>> 
>> 
>> Image name : image.ext
>> 
>> 
>> 
>> Scaled image names:
>> 
>> Windows : image.scale-dpiValue.ext
>> Linux : image.java-scale2x.ext
>> MAC image at 2x.ext
>> 
>> 
>> 
>> After the fix naming convention on Mac and Linux would be :
>> 
>> Image name : image.ext
>> 
>> Scaled image name : image.java-scale2x.ext
>> 
>>    Both name conventions @2x and java-scale2x should be supported on
>>    The more specific one @2x should be checked in the first place and the java-scale2x in the second.
>> 
>>   Thanks,
>>   Alexandr.
>> 
>> 
>> 
>> 
>> 
>> 
>> Naming convention on windows :
>> 
>> Image name : image.ext
>> 
>> Scaled image name : image.java-scale<dpi value>.ext
>> 
>> 
>> 
>> Regards,
>> 
>> Rajeev Chamyal
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
> 