<Swing Dev> [12] JDK-8182043: Access to Windows Large Icons

[Shashidhara Veerabhadraiah]

Hi Prashanta, Thanks for your review. Below are my replies:

 

http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.01/

 

Thanks and regards,
Shashi

 

From: Prasanta Sadhukhan 
Sent: Friday, July 20, 2018 3:42 PM
To: Shashidhara Veerabhadraiah <shashidhara.veerabhadraiah at oracle.com>; swing-dev at openjdk.java.net; awt-dev at openjdk.java.net
Subject: Re: <Swing Dev> [12] JDK-8182043: Access to Windows Large Icons

 

Hi Shashi,

The previous review thread is too long so probably need the original reviewer to take a look.
But here are my 2 cents

Can you please explain how the new API is supposed to be used? It says "Scaled icon for a file, directory, or folder as it would be displayed in a system file browser"

                getSystemIcon(File f, int width, int height)
            If the specified width, height is not available, then it will be scaled to available icon size, is it? I do not think the javadoc captures the essence of the api thoroughly.
            For example, if width/height asked for is 100 and we have icon of 96/96 and 128/128, then what it will return, the closest size 96/96 or the next positive one 128/128.

[Shashi] Yes. It will be scaled to the requested size. For the example that you mentioned, it is OS dependent. Typically it will pick up the higher resolution if available and scales it down to the requested level. If the higher resolution icon is not available then it will pick the lower one and scales it up. Hence I just happened to mention it as 'scaled icon' in the explanation.

As per fix of HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8151385"JDK-8151385, MAX_ICON_SIZE=128 was added so even if you accept width/height as 256 (by your check it will still restrict icon size to 128)...

        287         if((width > 256 || width < 1) || (height > 256 || height < 1)) {
        288             System.err.println("Warning: Icon scaling may be distorted");
        289             throw new IllegalArgumentException("unexpected icon scaling size");
        290         }

            so maybe either increase MAX_ICON_SIZE or adjust your check accordingly. 

[Shashi] The size I have kept is in a similar scale that is available with the OS. Though there is an internal limit of 128, code is present to scale it to 256 if the user wants in such a way.

We normally do not throw unchecked exception (RuntimeException) from public API, so you should consider throwing checked exception instead.

[Shashi] Fixed this.

Also, you need to change javadoc to have @throws instead of @exception

[Shashi] Fixed this.

Also, it was asked, IIUC that the public API used MutliResolutionImage and uses HYPERLINK "https://docs.oracle.com/javase/10/docs/api/java/awt/image/MultiResolutionImage.html#getResolutionVariants%28%29"getResolutionVariants() to select appropriate icon image or let user select it (for case mentioned above, if we are not sure ourselves)

[Shashi] Since the icon is scaled to the levels that was requested and limits are mentioned as part of Javadoc comments. Fixed in the updated Webrev.

.         scaleIconImage, scaleIcon uses lot of common code so we can have a common method

[Shashi] Fixed this.

makeIcon(long hIcon, int bsize).. I guess parameter name bsize does not convey meaning, is it "bestsize"?

[Shashi] Just an alternative and no meaning for that.

.         int size = getLargeIcon ? 32 : 16; and also in Win32ShellFolderManager2. java probably you can have constants for these 2 number
.         Win32ShellFolderManager2
.         1118                         return getShell32Icon(4, size);
.         1119                     } else {
.         1120                         return getShell32Icon(1, size);

You should use constant to be more readable FILE_ICON_ID = 1; FOLDER_ICON_ID = 4;

[Shashi] For the constants, I have newly added some comments explaining that magic number. Adding a new constant is not worth as it is used only once!!

Regards
Prasanta

On 7/20/2018 12:02 PM, Shashidhara Veerabhadraiah wrote:

Hi All, Please review a fix for the below bug.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8182043

 

Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.00/"http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.00/

 

History: This fix is derived from an earlier fix proposed under this mail thread: http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013016.html. Thanks to Semyon for that trial and part of this solution is continued from where it was left off.

 

Solution details: Large system icons were not able to be extracted because of sun package internal classes are not exposed anymore. This change adds a new api to get access to those icons.

 

Thanks and regards,

Shashi

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180730/a56be95b/attachment.html>