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

[Alexey Ivanov]

Hi Shashi,

Sorry for my belated review.

On 22/01/2019 18:03, Shashidhara Veerabhadraiah wrote:
>
> Hi All, Please review the below new fix compared to earlier webrevs. 
> Now the new api to get the icons of different sizes is being made part 
> of a new class SystemIcon. Please consider this as a different 
> solution than the one that was under review for a long time till now.
>
> http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.05/
>

This version looks cleaner.

The new SystemIcon class contains only one method, getSystemIcon. Does 
it justify creating a new class for this? Can't we add this extended 
version into FileSystemView class?

FileSystemView has method:
public Icon getSystemIcon(File f)

We're extending it with
public Icon getSystemIcon(File f, int size)

Are there any objections to this approach?


The class name, if we're going this route, could be more specific: 
FileSystemIcon seems a better alternative.


Javadoc for SystemIcon says, “SystemIcon is JFileChooser's gateway to 
the file system icons.” Yet JFileChooser uses 
FileSystemView.getSystemIcon(File). So Javadoc is not accurate at the 
very least.

There should also be @since tag for the class.

As this is a new API, I think it makes sense to throw 
IllegalArgumentException for invalid parameters: file == null or size is 
out of supported range.

You don't usually want to print exception stack trace or messages from a 
public API:
74             System.err.println("FileSystemView.getShellFolder: f="+f);
75             e.printStackTrace();

As Phil mentioned, you should clearly state when a null value can be 
returned.


static public Icon getSystemIcon(File f, int size)
should be
public static Icon getSystemIcon(File f, int size)

At this time, SystemIcon is an utility class which provides only static 
methods. Yet it can be instantiated which does not look good.
The code indents by three spaces instead of four.

Win32ShellFolder2.java
1111     public Image getIcon(int size) {
The method should have @Override annotation.

It can also be added to
1040     public Image getIcon(final boolean getLargeIcon) {


Win32ShellFolderManager2.java
383                          key.startsWith("shell32LargeIcon 
")?LARGE_ICON_SIZE : SMALL_ICON_SIZE);

There should be spaces around ‘?’.


> Part of the fix is being borrowed from Semyon’s fix and would like to 
> thank Semyon for that.
>

You can give Semyon attribution by including "Contributed-by" tag in 
your commit message and referencing Semyon and yourself there.


Regards,
Alexey

> Thanks and regards,
>
> Shashi
>
> <SNIP>