Hi Alexey, Thanks for your review and below is the new Webrev.
http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/
Please see below for inline comments.
Thanks and regards,
Shashi
From: Alexey Ivanov
Sent: Friday, September 21, 2018 2:09 PM
To: Shashidhara Veerabhadraiah <shashidhara.veerabhadra...@oracle.com>;
Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>; swing-dev
<swing-dev@openjdk.java.net>; awt-dev <awt-...@openjdk.java.net>
Subject: Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to Windows Large
Icons
Hi Shashi,
SystemIcon.java
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?
[Shashi] SystemIcon is going to be the front face to access the icons and that
is the purpose of this class. The reason for choosing this is that
FileSystemView class can be used internally and did not wanted to expose it
externally too. Externally exposing may cause certain restriction in
maintaining the classes hence the indirection.
FileSystemView.java
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).
[Shashi] Because as you said rightly it follows the 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.
[Shashi] User has no restriction of mentioning any size but the platform may
have a limitation of size. Since we are supporting a set of different versions
of platforms, platform may limit the size of the icon to a particular size, in
which case it will be scaled to the user requested size.
270 protected Icon getSystemIcon(File f, int size) {
Can't the method be public? It was in Semyon's review.
[Shashi] Because of the indirection, this method can stay as protected. I think
it is always good to be of using protected than making everything public. Also
that is the advantage of adding the SystemIcon class.
266 * @return an icon as it would be displayed by a native file chooser
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?
[Shashi] Done. I can only say that 0 does not make sense. Check is to see that
it is not less than 1.
ShellFolder.java
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) {
[Shashi] Updated. Thank you.
ShellFolder2.cpp
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.
[Shashi] Updated.
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.
[Shashi] Per me, the problem is that since we support certain older versions of
the platforms, it should not cause an exception at the native level. If
everyone agrees for the change then we can change that.
Win32ShellFolder2.java
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.
[Shashi] Updated.
1031 int size = getLargeIcon ? 32 : 16; // large icon is 32 pixels else
16 pixels
Create constants for these magic numbers.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java.udiff.html
[Shashi] Updated.
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.
[Shashi] But my view is that if we start doing for every immediate value, then
creation of the object and assignment may have some implications. If it is used
more than once, yes definitely create the constant. Please let me know if you
think otherwise. This I follow it as a standard by practice.
1113 if(hIcon <= 0) {
1116 if(hIcon <= 0) {
Please add space between if and the opening parenthesis.
[Shashi] Updated.
Win32ShellFolderManager2.java
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.*;
[Shashi] Updated.
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);
See
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html
SystemIconTest.java
Could you please order the imports? Your IDE would be happy to do it for you.
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.
[Shashi] Updated.
Regards,
Alexey
On 04/09/2018 10:39, Shashidhara Veerabhadraiah wrote:
Hi All, Please find the updated Webrev per the discussion:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.02/"http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.02/
Thanks and regards,
Shashi
From: Shashidhara Veerabhadraiah
Sent: Monday, July 30, 2018 1:32 PM
To: Prasanta Sadhukhan HYPERLINK
"mailto:prasanta.sadhuk...@oracle.com";<prasanta.sadhuk...@oracle.com>;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net;
HYPERLINK "mailto:awt-...@openjdk.java.net"awt-...@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:
HYPERLINK
"http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.01/"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 <HYPERLINK
"mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>;
HYPERLINK "mailto:swing-dev@openjdk.java.net"swing-dev@openjdk.java.net;
HYPERLINK "mailto:awt-...@openjdk.java.net"awt-...@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
