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

[Phil Race]

Alexey,

We already nixed this approach
See https://mail.openjdk.java.net/pipermail/awt-dev/2019-January/014972.html

Shashi is re-thinking it to use MultiResolutionImage.

-phil.

On 3/1/19 12:25 PM, Alexey Ivanov wrote:
> 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>
>