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
<[email protected]> 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
<[email protected]> 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.
