[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-awt-dev
Subject: Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to Windows Large Icons
From: Shashidhara Veerabhadraiah <shashidhara.veerabhadraiah () oracle ! com>
Date: 2018-09-28 8:58:43
Message-ID: a706ab54-6054-4954-a369-d35986f1a19f () default
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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/
Thanks and regards,
Shashi
From: Alexey Ivanov
Sent: Tuesday, September 25, 2018 3:00 AM
To: Shashidhara Veerabhadraiah <shashidhara.veerabhadraiah@oracle.com>; Prasanta \
Sadhukhan <prasanta.sadhukhan@oracle.com>; swing-dev <swing-dev@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,
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.
HYPERLINK "http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.03/"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 HYPERLINK \
"mailto:shashidhara.veerabhadraiah@oracle.com"<shashidhara.veerabhadraiah@oracle.com>; \
Prasanta Sadhukhan HYPERLINK \
"mailto:prasanta.sadhukhan@oracle.com"<prasanta.sadhukhan@oracle.com>; swing-dev \
HYPERLINK "mailto:swing-dev@openjdk.java.net"<swing-dev@openjdk.java.net>; awt-dev \
HYPERLINK "mailto:awt-dev@openjdk.java.net"<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.
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.
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.
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.
65 public SystemIcon() {
66 this.file = null;
67 }
Should rather call this(null) constructor.
[Shashi] Correct and updated.
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!!
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.
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.
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
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.
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
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
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
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.
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.
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.
HYPERLINK "http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.01/src/java.desktop/ \
windows/classes/sun/awt/shell/Win32ShellFolder2.java.udiff.html"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.
Primitive type constants do not have any performance penalty as they're resolved at \
compile time. There's no difference if you write getShell32Icon(FOLDER_ICON_INDEX, \
size) or getShell32Icon(4, size). Yet the former is much clearer, isn't it?
With the constants, the if expression could be replaced with the conditional \
operator:
if(hIcon <= 0) {
return getShell32Icon(isDirectory() ? FOLDER_ICON_INDEX : FILE_ICON_INDEX, size);
}
I agree not every number in code should be defined as a constant. 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.
1138 Image icon = makeIcon(hIcon, 32);
Now should use LARGE_ICON_SIZE.
[Shashi] Updated
1113 if(hIcon <= 0) {
1116 if(hIcon <= 0) {
Please add space between if and the opening parenthesis.
[Shashi] Updated.
1078 if(hIcon <= 0) {
1081 if(hIcon <= 0) {
These (in the method above) could also be 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.
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
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 HYPERLINK "http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.01/src/java.desk \
top/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html"http://cr.o \
penjdk.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.
Could you please organize imports?
There are only three classes used.
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
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
<SNIP>
[Attachment #5 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=iso-8859-1"><meta name=Generator content="Microsoft Word \
15 (filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:PMingLiU;
panose-1:2 1 6 1 0 1 1 1 1 1;}
@font-face
{font-family:Tunga;
panose-1:0 0 4 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"\@PMingLiU";
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
code
{mso-style-priority:99;
font-family:"Courier New";}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.new
{mso-style-name:new;}
span.membernamelink
{mso-style-name:membernamelink;}
span.changed
{mso-style-name:changed;}
span.EmailStyle25
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle26
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle27
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle28
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle29
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle30
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle31
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body bgcolor=white lang=EN-US \
link="#0563C1" vlink="#954F72"><div class=WordSection1><p class=MsoNormal><span \
style='color:#1F497D'>Hi Alexey, Thank you for your thorough review. I have updated \
the copyrights as well and please see below for my comments:<o:p></o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'>Here is the new Webrev: <a \
href="http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.04/">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.04/</a><o:p></o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'><o:p> </o:p></span></p><p \
class=MsoNormal><span style='color:#1F497D'>Thanks and \
regards,<o:p></o:p></span></p><p class=MsoNormal><span \
style='color:#1F497D'>Shashi<o:p></o:p></span></p><p class=MsoNormal><a \
name="_MailEndCompose"><span \
style='color:#1F497D'><o:p> </o:p></span></a></p><div><div \
style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span style='color:windowtext'>From:</span></b><span \
style='color:windowtext'> Alexey Ivanov <br><b>Sent:</b> Tuesday, September 25, 2018 \
3:00 AM<br><b>To:</b> Shashidhara Veerabhadraiah \
<shashidhara.veerabhadraiah@oracle.com>; Prasanta Sadhukhan \
<prasanta.sadhukhan@oracle.com>; swing-dev <swing-dev@openjdk.java.net>; \
awt-dev <awt-dev@openjdk.java.net><br><b>Subject:</b> Re: <AWT Dev> \
<Swing Dev> [12] JDK-8182043: Access to Windows Large \
Icons<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><p \
class=MsoNormal style='margin-bottom:12.0pt'>Hi Shashi,<br><br>Please see my comments \
inline:<span style='font-size:12.0pt'><o:p></o:p></span></p><div><p \
class=MsoNormal>On 21/09/2018 23:22, Shashidhara Veerabhadraiah \
wrote:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal><span \
style='color:#1F497D'>Hi Alexey, Thanks for your review and below is the new \
Webrev.</span><o:p></o:p></p><p class=MsoNormal><span style='color:#1F497D'><a \
href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.03/">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.03/</a></span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'>Please see below for inline \
comments.</span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'> </span><o:p></o:p></p><p class=MsoNormal><span \
style='color:#1F497D'>Thanks and regards,<br>Shashi</span><o:p></o:p></p><p \
class=MsoNormal><span style='color:#1F497D'> </span><o:p></o:p></p><div><div \
style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span style='color:windowtext'>From:</span></b><span \
style='color:windowtext'> Alexey Ivanov <br><b>Sent:</b> Friday, September 21, 2018 \
2:09 PM<br><b>To:</b> Shashidhara Veerabhadraiah <a \
href="mailto:shashidhara.veerabhadraiah@oracle.com"><shashidhara.veerabhadraiah@oracle.com></a>; \
Prasanta Sadhukhan <a \
href="mailto:prasanta.sadhukhan@oracle.com"><prasanta.sadhukhan@oracle.com></a>; \
swing-dev <a href="mailto:swing-dev@openjdk.java.net"><swing-dev@openjdk.java.net></a>; \
awt-dev <a href="mailto:awt-dev@openjdk.java.net"><awt-dev@openjdk.java.net></a><br><b>Subject:</b> \
Re: <AWT Dev> <Swing Dev> [12] JDK-8182043: Access to Windows Large \
Icons</span><o:p></o:p></p></div></div><p class=MsoNormal> <o:p></o:p></p><p \
class=MsoNormal style='margin-bottom:12.0pt'>Hi \
Shashi,<br><br>SystemIcon.java<br>What is the purpose of new SystemIcon \
class?<br>It's not used anywhere but the provided test. Is this class really needed \
then?<br>Is it supposed to become the public API for accessing system icons?<br>Why \
can't FileSystemView be used for that purpose as it was proposed in Semyon's \
review?<br><b><i><span style='color:#1F497D'>[Shashi] </span></i></b><span \
style='color:#1F497D'>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.</span><o:p></o:p></p></blockquote><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br>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).<br>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.<br><br>It can be \
made public to facilitate access to file icons.<br>After all, protected method is \
also a contract, it cannot be changed without affecting backward \
compatibility.<br><br>It is this new protected method that performs the task of \
getting the icon from the system.<br><br>Do we really need other methods?</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[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.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><b><i><span \
style='color:#1F497D'><o:p> </o:p></span></i></b></p><p \
class=MsoNormal><b><i><span style='color:#1F497D'>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.<o:p></o:p></span></i></b></p><p class=MsoNormal><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br>278 public File \
createNewFolder(File containingDir) throws IOException \
{<br>279 return \
null;<br>280 }<br>You had to implement the abstract method \
from FileSystemView. It's one more point to make system icon available right from \
FileSystemView.<br>This implementation should rather throw an exception.</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[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.<o:p></o:p></span></i></b></p><p class=MsoNormal><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br>60 \
protected File file;<br>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.<br>(The field \
could be final as it cannot be changed and should not be changed.)</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br>65 public SystemIcon() \
{<br>66 this.file = \
null;<br>67 }<br>Should rather call this(null) \
constructor.</span><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Correct and updated.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br>112 public Icon getSystemIcon(int \
width, int height) {<br>Are methods with width / height parameters needed? Icons are \
usually square.</span><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[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!!<o:p></o:p></span></i></b></p><p class=MsoNormal><span \
style='font-size:12.0pt;font-family:"Times New Roman",serif'><br><br>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.</span><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br>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.</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br>210 int \
size;<br>211 if(width > height) \
{<br>212 size \
= width;<br>213 } else \
{<br>214 size \
= height;<br>215 }<br>This code can \
be simplified to<br>int size = Math.max(width, height);<br>Concise and clear.<br>A \
helper method which validates the parameters could also return this value. Thus, \
again, avoiding code duplication among many methods in this class.</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br>There are lots of tabs in this file. Tabs must be replaced \
with spaces.<br>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.<br>Please also sort the imports and remove unused ones.</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br><br><o:p></o:p></span></p><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal \
style='margin-bottom:12.0pt'>FileSystemView.java<br> 259 \
* Icon for a file, directory, or folder as it would be displayed \
in<br> 260 * a system file browser for the \
requested size.<br>For getXXX, it's better to start description with \
“Returns…” so it aligns to other similar methods.<br>However, I see \
the new method follows description of getIcon(boolean).<o:p></o:p></p><p \
class=MsoNormal style='margin-bottom:12.0pt'><b><i><span \
style='color:#1F497D'>[Shashi] </span></i></b><span style='color:#1F497D'>Because as \
you said rightly it follows the getIcon(boolean)</span><o:p></o:p></p></blockquote><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br>Okay.<br>Is it possible to update documentation to the existing \
getSystemIcon(File)?<br>Should I file a separate bug to update the \
documentation?<br><br>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.</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated.<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br><o:p></o:p></span></p><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal \
style='margin-bottom:12.0pt'> 265 * @param size \
width and height of the icon in pixels to be scaled(valid range: 1 to 256)<br>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.<o:p></o:p></p><p class=MsoNormal \
style='margin-bottom:12.0pt'><b><i><span style='color:#1F497D'>[Shashi] \
</span></i></b><span style='color:#1F497D'>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.</span><o:p></o:p></p></blockquote><p class=MsoNormal><span \
style='font-size:12.0pt;font-family:"Times New Roman",serif'><br>I understand that. \
However, I think the suggested description does not convey the meaning \
correctly.<br>The method will return the icon of the requested size, won't it?<br>So \
the correct description is:<br>@param size width and height of the icon in pixels \
(valid range: 1 to 256)<br><br>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:<br>@return an icon of the requested size (possibly scaled) as it \
would be displayed by a native file chooser</span><span \
style='font-size:12.0pt;font-family:"Times New \
Roman",serif;color:#1F497D'><o:p></o:p></span></p><p class=MsoNormal><b><i><span \
style='color:#1F497D'>[Shashi] Updated<o:p></o:p></span></i></b></p><p \
class=MsoNormal><span style='font-size:12.0pt;font-family:"Times New \
Roman",serif'><br><br><br><o:p></o:p></span></p><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal \
style='margin-bottom:12.0pt'>270 protected Icon \
["winmail.dat" (application/ms-tnef)]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic