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

Alexey Ivanov alexey.ivanov at oracle.com
Fri Sep 21 21:09:18 UTC 2018

Hi Shashi,

What is the purpose of new SystemIcon class?
It's not used anywhere but the provided test. Is this class really 
needed then?
Is it supposed to become the public API for accessing system icons?
Why can't FileSystemView be used for that purpose as it was proposed in 
Semyon's review?

  259      * Icon for a file, directory, or folder as it would be 
displayed in
  260      * a system file browser for the requested size.
For getXXX, it's better to start description with “Returns…” so it 
aligns to other similar methods.
However, I see the new method follows description of getIcon(boolean).

  265      * @param size width and height of the icon in pixels to be 
scaled(valid range: 1 to 256)
Why is it “to be scaled”? I would expect to get the icon of the 
requested size. At the same time, the icon can be scaled to the 
requested size if the requested size is not available.

270     protected Icon getSystemIcon(File f, int size) {
Can't the method be public? It was in Semyon's review.

  266      * @return an icon as it would be displayed by a native file 
An icon of the requested size (possibly scaled) as…

  275         if(size > 256 || size < 1) {
  276             return null;
  277         }
Please add space between if and the opening parenthesis.
You can throw InvalidArgumentException in this case.
Does size of 1 make any sense?

  202     /**
  203      * @param size size of the icon > 0
  204      * @return The icon used to display this shell folder
  205      */
Can you add a short description of the purpose of this method? “Returns 
the icon of the specified size used to display this shell folder”?
A similar description can be added to the method above it:
198     public Image getIcon(boolean getLargeIcon) {

  944             hres = pIcon->Extract(szBuf, index, &hIcon, 0, size);
Please use NULL instead of 0. This way it's clear you pass a null 
pointer rather an integer with value of 0.

974     const int MAX_ICON_SIZE = 128;
I also suggest increasing MAX_ICON_SIZE to 256. Otherwise I see no point 
in allowing 256 as the maximum size at Java level as you'll never have 
icon of 256×256 even thought the system may have one.

1007     private static Image makeIcon(long hIcon, int bsize) {
bsize has no meaning. Prasanta also asked about the name of the parameter.
I suggest using "size" for parameter, and "iconSize" for local variable.

1031         int size = getLargeIcon ? 32 : 16; // large icon is 32 
pixels else 16 pixels
Create constants for these magic numbers.

1080                                         return getShell32Icon(4, 
size); // pick folder icon
1081                                     } else {
1082                                         return getShell32Icon(1, 
size); // pick file icon
Also create constants for 4 and 1 as Prasanta suggested.
Creating a constant costs you nothing but makes the code more readable 
without adding comments.

1113             if(hIcon <= 0) {
1116                 if(hIcon <= 0) {
Please add space between if and the opening parenthesis.

  382                     return Win32ShellFolder2.getShell32Icon(i, 
key.startsWith("shell32LargeIcon ")?
  383 32 : 16);
You can use constants declared for icon size in from Win32ShellFolder2 
because they're already imported:
   42 import static sun.awt.shell.Win32ShellFolder2.*;

Then the code at these lines needs to be updated too:
  129             STANDARD_VIEW_BUTTONS[iconIndex] = (size == 16)
  130                     ? img
  131                     : new MultiResolutionIconImage(16, img);



Could you please order the imports? Your IDE would be happy to do it for 
Could you also add spaces between if and the opening parenthesis?
(Or at least be consistent with it.)

58             ImageIcon icon = (ImageIcon)sysIcon.getSystemIcon(file, 
size, size);
Casts should be followed by a blank space.

67             else if (icon.getIconWidth() != size) {
else is redundant as the preceding if throws an exception.


On 04/09/2018 10:39, Shashidhara Veerabhadraiah wrote:
> Hi All, Please find the updated Webrev per the discussion:
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.02/ 
> <http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.02/>
> Thanks and regards,
> Shashi
> *From:*Shashidhara Veerabhadraiah
> *Sent:* Monday, July 30, 2018 1:32 PM
> *To:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com>; 
> swing-dev at openjdk.java.net; awt-dev at openjdk.java.net
> *Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to 
> Windows Large Icons
> Hi Prashanta, Thanks for your review. Below are my replies:
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.01/ 
> <http://cr.openjdk.java.net/%7Esveerabhadra/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 
> <mailto:shashidhara.veerabhadraiah at oracle.com>>; 
> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>; 
> awt-dev at openjdk.java.net <mailto: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 /JDK-8151385/
>     <https://bugs.openjdk.java.net/browse/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 getResolutionVariants
>     <https://docs.oracle.com/javase/10/docs/api/java/awt/image/MultiResolutionImage.html#getResolutionVariants%28%29>|()
>     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; 
> */[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:
>     http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.00/
>     <http://cr.openjdk.java.net/%7Esveerabhadra/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/awt-dev/attachments/20180921/d9b1ae9a/attachment-0001.html>

More information about the awt-dev mailing list