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/
Part of the fix is being borrowed from Semyon’s fix and would like to
thank Semyon for that.
Thanks and regards,
Shashi
*From:*Alexey Ivanov
*Sent:* Thursday, October 11, 2018 4:32 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>; Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>; swing-dev
<swing-...@openjdk.java.net>; awt-dev <awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to
Windows Large Icons
Hi Shashi,
Thank you for updating the review.
With updated copyright years, the patch does not apply cleanly because
some files already have 2018. It's a minor nuisance which could be
easily resolved.
Other comments inline:
On 28/09/2018 09:58, Shashidhara Veerabhadraiah wrote:
Hi Alexey, Thank you for your thorough review. I have updated the
copyrights as well and please see below for my comments:
Here is the new Webrev:
http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.04/
<http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.04/>
Thanks and regards,
Shashi
*From:*Alexey Ivanov
*Sent:* Tuesday, September 25, 2018 3:00 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>; Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>; swing-dev
<swing-...@openjdk.java.net> <mailto:swing-...@openjdk.java.net>;
awt-dev <awt-dev@openjdk.java.net> <mailto:awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to
Windows Large Icons
Hi Shashi,
Please see my comments inline:
On 21/09/2018 23:22, Shashidhara Veerabhadraiah wrote:
Hi Alexey, Thanks for your review and below is the new Webrev.
http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/
<http://cr.openjdk.java.net/%7Esveerabhadra/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>
<mailto:shashidhara.veerabhadra...@oracle.com>; Prasanta
Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>; swing-dev
<swing-...@openjdk.java.net>
<mailto:swing-...@openjdk.java.net>; awt-dev
<awt-dev@openjdk.java.net> <mailto:awt-dev@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.
Still, I cannot understand the rationale for a new class the only
purpose of which is to provide public access to
getSystemIcon(File, int, int).
FileSystemView is already a public class, and it's used
internally. (I guess it would not have existed, if it hadn't.) It
has a public method getSystemIcon(File). As such, extending its
functionality to get access to larger icons seems logical. This is
what the new protected getSystemIcon(File f, int size) does.
It can be made public to facilitate access to file icons.
After all, protected method is also a contract, it cannot be
changed without affecting backward compatibility.
It is this new protected method that performs the task of getting
the icon from the system.
Do we really need other methods?
*/[Shashi] I think that system icons functions as part of
filesystemview class is also a kind of corrupted creation of the
filesystemview class. Icons forms a different functionality
compared to file system and should have been kept as a separate
class in the first place./*
I agree to some extent… Yet FileSystemView.getSystemIcon(File) is part
of this class since 1.4. Having this in mind, I see no reason why an
extended version getSystemIcon(File file, int size) cannot be public.
If the new method is public, the access to large icons, or rather
icons of arbitrary size, is provided.
Semyon's review made the new method public:
http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013016.html
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
and the latest version
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/
I'm still not convinced the new method should be exposed via a new
class SystemIcon. I see no advantages to this compared to making the
new method public in FileSystemView.
The fact that you have to implement the abstract method
createNewFolder() from FileSystemView for which you cannot provide a
reasonable implementation only emphasizes it's a bad design decision.
*/Regarding the methods, am not sure does it required or not but I
added them to be complete. Some variant is required to provide the
original unmodified icon as the system returns and provide the
scaled icon as well./*
I don't think they're needed.
Other helper methods merely scale the icon fetched via
FileSystemView.getSystemIcon(File file, int size). Such scaling is not
unique to icons, it's applicable to any images. As such, they should
be in a dedicated utility class, shouldn't they?
278 public File createNewFolder(File containingDir) throws
IOException {
279 return null;
280 }
You had to implement the abstract method from FileSystemView. It's
one more point to make system icon available right from
FileSystemView.
This implementation should rather throw an exception.
*/[Shashi] IOException? I think it’s not needed as we won’t be
performing any such actions. But yes I had to implement this place
holder to be complete./*
That's exactly the problem. It's a public method which can be called
by whoever wants to do it, and the caller rightfully expects the
method to fulfil its contract. But this implementation can never
fulfil its contract.
It is the reason why I think SystemIcon class is not the way to go.
60 protected File file;
This field is redundant, in my opinion. It would be quite
expensive to instantiate SystemIcon object for each file. It can
safely be removed, then only methods which take additional File
parameter will be left.
(The field could be final as it cannot be changed and should not
be changed.)
*/[Shashi] Updated./*
Thanks!
Yet my main point was that you can safely remove this field and then
leave only methods which take it as parameter.
<SNIP>
112 public Icon getSystemIcon(int width, int height) {
Are methods with width / height parameters needed? Icons are
usually square.
*/[Shashi] Flexibility is ok I think. It would fall back to the
same function though. Though the native does not have the function
and since because of scaling we can support that. Am not sure
where it can be useful but being flexible is ok I think!!/*
Flexibility is okay unless it's flexibility for the sake of flexibility.
On Windows, icons are always square. The new API is system
independent. So the method makes sense if and only if there are
platforms where file icons can be non-square.
Can file icon have different width and height on Linux or macOS? If
not, I'm for dropping the method.
You repeat checks if f is null, width and height checks in each
and every method. I guess parameter validation could be extract
into a separate method. You will avoid lots of cope duplication.
*/[Shashi] Updated/*
Since it's a completely new API, I suggest throwing
IllegalArgumentException with appropriate message in the cases
where a parameter (file, width and height) fails validation.
*/[Shashi] Updated./*
Not all documentation comments for the public methods include throws
clauses for the thrown IllegalArgumentException.
210 int size;
211 if(width > height) {
212 size = width;
213 } else {
214 size = height;
215 }
This code can be simplified to
int size = Math.max(width, height);
Concise and clear.
A helper method which validates the parameters could also return
this value. Thus, again, avoiding code duplication among many
methods in this class.
*/[Shashi] Updated./*
I don't think getSize is a good name for the helper method.
It could be validateParameters, thus its purpose is clearer.
Documentation comment could clarify it returns the size of icon based
on width and height.
The method should be static as it does not use any fields.
There are lots of tabs in this file. Tabs must be replaced with
spaces.
if's are inconsistent throughout the code: some are with space,
some are without. Please add the space everyone to align with Java
Code Conventions.
Please also sort the imports and remove unused ones.
*/[Shashi] Updated/*
The imports are not sorted.
There are three unused imports.
259 return (Image)scaledImage;
The cast is redundant.
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)
Okay.
Is it possible to update documentation to the existing
getSystemIcon(File)?
Should I file a separate bug to update the documentation?
Documentation also references a non-public class ShellFolder.
Should this reference be removed from documentation as the access
to non-public classes is restricted? It does not add much value.
*/[Shashi] Updated./*
The documentation for the newly added method hasn't been updated.
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.
I understand that. However, I think the suggested description does
not convey the meaning correctly.
The method will return the icon of the requested size, won't it?
So the correct description is:
@param size width and height of the icon in pixels (valid range: 1
to 256)
The fact the returned icon may be scaled if the requested size is
not available must be described in the method documentation as
well as in @return line:
@return an icon of the requested size (possibly scaled) as it
would be displayed by a native file chooser
*/[Shashi] Updated/*
I can't see any change here.
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.
Sorry I don't see any advantage of having SystemIcon class over
making this method public as I outlined above.
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.
What about throwing InvalidArgumentException when size parameter
is invalid?
*/[Shashi] Updated/*
Not updated:
273 if (size > 256 || size < 1) {
274 return null;
275 }
The method silently returns null as before.
288 return UIManager.getIcon(f.isDirectory() ?
"FileView.directoryIcon" : "FileView.fileIcon");
Should the icon be returned as MultiResolutionImage if icon size is
different from the requested size?
I understand that check is to make sure size is at least 1.
However, icon of 1 pixel size does not make any sense. Should the
minimum be a more sensible of 4?
It's a concern for discussion.
*/[Shashi] /*On the native side, there is no restriction so I
think we can keep this open.
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.
Thank you for updating @return clause of the Javadoc.
My intention was to add a generic description of the method as well:
202 /**
202 * Returns the icon of the specified size used to display
this shell folder.
202 *
203 * @param size size of the icon > 0
204 * @return The icon used to display this shell folder
205 */
Such description could also be added to method above
getIcon(boolean getLargeIcon), at line 198.
Should the range of size parameter be specified? For example,
1–256 as in FileSystemView.
*/[Shashi] /*Updated
207 * @param size size of the icon > 0(Valid range: 1 to 256)
Isn't >0 redundant now?
@param size size of the icon, valid range from 1 to 256
looks better, doesn't it?
<SNIP>
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.
This concern was raised in the previous review too:
http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013115.html
I think it's safe to update the value of MAX_ICON_SIZE to 256. The
oldest supported version of Windows is Windows 7 which supports
256×256 icons.
Windows XP used icons up to 48×48, but it does not imply the API
does not allow loading icon of larger size. Both 128 and 256
should be tested on Windows XP if JDK still runs on it.
*/[Shashi] /*I will raise a bug on this and work on it later. I
have to see fn_GetIconInfo() and what it returns the values and
based on it, it is good to update to 256 I think.
Why can't this be done under this bug?
We promise to return icon of size 256 from the system but we will
never return icon larger than 128.
fn_GetIconInfo() is GetIconInfo from Windows API, see
202 fn_GetIconInfo = (GetIconInfoType)GetProcAddress(libUser32,
"GetIconInfo");
https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-geticoninfo
https://docs.microsoft.com/en-ie/windows/desktop/api/winuser/ns-winuser-_iconinfo
The function returns HBITMAP, the bitmap data for the icon. Its size
is used to calculate the size of the icon.
If we requested icon of size 256, the size of the bitmap would be 256×256.
Win32ShellFolder2.java
<SNIP>
… Yet I'm for using constants in this particular piece of code.
These values are used at least twice in
Win32ShellFolder2.java: lines 1081–1085 and 1119–1123.
*/[Shashi] Updated./*
These could private:
80 static final int FILE_ICON_ID = 1;
81 static final int FOLDER_ICON_ID = 4;
<SNIP>
382 return
Win32ShellFolder2.getShell32Icon(i,
key.startsWith("shell32LargeIcon ")?
383 LARGE_ICON_SIZE : SMALL_ICON_SIZE);
May I suggest updating formatting to:
return Win32ShellFolder2.getShell32Icon(i,
key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE :
SMALL_ICON_SIZE);
or even
return Win32ShellFolder2.getShell32Icon(i,
key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE
:
SMALL_ICON_SIZE);
(where : aligns with ?)
*/[Shashi] /*Updated
383 key.startsWith("shell32LargeIcon ")?LARGE_ICON_SIZE :
SMALL_ICON_SIZE);
Spaces around ? please.
<SNIP>
SystemIconTest.java
Could you please organize imports?
There are only three classes used.
The imports are usually sorted.
Is it possible to move java.io.File to the top of the list?
(Your IDE can do it for you.)
Regards,
Alexey
41 System.out.println("Windows detected: will run
sytem icons test");
typo: system
Since the test is Windows-specific, it can be declared using
@requires tag of JTreg:
@requires os.family == "windows"
*/[Shashi] /*Updated.
Regards,
Alexey
