<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
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>
<br>
<br>
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).<br>
<br>
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.<br>
<br>
270 protected Icon getSystemIcon(File f, int size) {<br>
Can't the method be public? It was in Semyon's review.<br>
<br>
266 * @return an icon as it would be displayed by a native
file chooser<br>
An icon of the requested size (possibly scaled) as…<br>
<br>
275 if(size > 256 || size < 1) {<br>
276 return null;<br>
277 }<br>
Please add space between if and the opening parenthesis.<br>
You can throw InvalidArgumentException in this case.<br>
Does size of 1 make any sense?<br>
<br>
<br>
ShellFolder.java<br>
202 /**<br>
203 * @param size size of the icon > 0<br>
204 * @return The icon used to display this shell folder<br>
205 */<br>
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”?<br>
A similar description can be added to the method above it:<br>
198 public Image getIcon(boolean getLargeIcon) {<br>
<br>
<br>
ShellFolder2.cpp<br>
944 hres = pIcon->Extract(szBuf, index, &hIcon,
0, size);<br>
Please use NULL instead of 0. This way it's clear you pass a null
pointer rather an integer with value of 0.<br>
<br>
974 const int MAX_ICON_SIZE = 128;<br>
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.<br>
<br>
<br>
Win32ShellFolder2.java<br>
1007 private static Image makeIcon(long hIcon, int bsize) {<br>
bsize has no meaning. Prasanta also asked about the name of the
parameter.<br>
I suggest using "size" for parameter, and "iconSize" for local
variable.<br>
<br>
1031 int size = getLargeIcon ? 32 : 16; // large icon is 32
pixels else 16 pixels<br>
Create constants for these magic numbers.<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ssadetsky/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</a><br>
<br>
1080 return
getShell32Icon(4, size); // pick folder icon<br>
1081 } else {<br>
1082 return
getShell32Icon(1, size); // pick file icon<br>
Also create constants for 4 and 1 as Prasanta suggested.<br>
Creating a constant costs you nothing but makes the code more
readable without adding comments.<br>
<br>
1113 if(hIcon <= 0) {<br>
1116 if(hIcon <= 0) {<br>
Please add space between if and the opening parenthesis.<br>
<br>
<br>
Win32ShellFolderManager2.java<br>
382 return Win32ShellFolder2.getShell32Icon(i,
key.startsWith("shell32LargeIcon ")?<br>
383
32 : 16);<br>
You can use constants declared for icon size in from
Win32ShellFolder2 because they're already imported:<br>
42 import static sun.awt.shell.Win32ShellFolder2.*;<br>
<br>
Then the code at these lines needs to be updated too:<br>
129 STANDARD_VIEW_BUTTONS[iconIndex] = (size == 16)<br>
130 ? img<br>
131 : new MultiResolutionIconImage(16, img);<br>
<br>
See
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java.udiff.html</a><br>
<br>
<br>
SystemIconTest.java<br>
<br>
Could you please order the imports? Your IDE would be happy to do it
for you.<br>
Could you also add spaces between if and the opening parenthesis?<br>
(Or at least be consistent with it.)<br>
<br>
58 ImageIcon icon =
(ImageIcon)sysIcon.getSystemIcon(file, size, size);<br>
Casts should be followed by a blank space.<br>
<br>
67 else if (icon.getIconWidth() != size) {<br>
else is redundant as the preceding if throws an exception.<br>
<br>
<br>
Regards,<br>
Alexey<br>
<br>
<br>
<div class="moz-cite-prefix">On 04/09/2018 10:39, Shashidhara
Veerabhadraiah wrote:<br>
</div>
<blockquote type="cite"
cite="mid:378efc00-ae1a-4e9c-861e-e563657486ce@default">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@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";}
/* 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-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
.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;}
/* List Definitions */
@list l0
{mso-list-id:901988770;
mso-list-template-ids:1374585078;}
@list l0:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l0:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level4
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level5
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level7
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level8
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l0:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1
{mso-list-id:1398866647;
mso-list-template-ids:864182752;}
@list l1:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l1:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l1:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level4
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level5
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level7
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level8
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l1:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2
{mso-list-id:1511217915;
mso-list-template-ids:1018584036;}
@list l2:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l2:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l2:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level4
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level5
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level7
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level8
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l2:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3
{mso-list-id:1867256503;
mso-list-template-ids:971115580;}
@list l3:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l3:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l3:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level4
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level5
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level7
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level8
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l3:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4
{mso-list-id:1921719012;
mso-list-template-ids:-1036637156;}
@list l4:level1
{mso-level-number-format:bullet;
mso-level-text:\F0B7;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l4:level2
{mso-level-number-format:bullet;
mso-level-text:o;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:"Courier New";
mso-bidi-font-family:"Times New Roman";}
@list l4:level3
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level4
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level5
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level6
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level7
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level8
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
@list l4:level9
{mso-level-number-format:bullet;
mso-level-text:\F0A7;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Wingdings;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></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]-->
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D">Hi All, Please
find the updated Webrev per the discussion:<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"><a
href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.02/</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"
moz-do-not-send="true"><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"> Shashidhara Veerabhadraiah <br>
<b>Sent:</b> Monday, July 30, 2018 1:32 PM<br>
<b>To:</b> Prasanta Sadhukhan
<a class="moz-txt-link-rfc2396E" href="mailto:prasanta.sadhukhan@oracle.com"><prasanta.sadhukhan@oracle.com></a>;
<a class="moz-txt-link-abbreviated" href="mailto:swing-dev@openjdk.java.net">swing-dev@openjdk.java.net</a>; <a class="moz-txt-link-abbreviated" 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<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="color:#1F497D">Hi Prashanta,
Thanks for your review. Below are my replies:<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"><a
href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.01/</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,<br>
Shashi<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></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"> Prasanta Sadhukhan <br>
<b>Sent:</b> Friday, July 20, 2018 3:42 PM<br>
<b>To:</b> Shashidhara Veerabhadraiah <<a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>>;
<a href="mailto:swing-dev@openjdk.java.net"
moz-do-not-send="true">swing-dev@openjdk.java.net</a>;
<a href="mailto:awt-dev@openjdk.java.net"
moz-do-not-send="true">awt-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: <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>Hi Shashi,<span style="font-size:12.0pt"><o:p></o:p></span></p>
<p class="MsoNormal">The previous review thread is too long so
probably need the original reviewer to take a look.<br>
But here are my 2 cents<o:p></o:p></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l3
level1 lfo1">Can you please explain how the new API is
supposed to be used? It says "<span class="new">Scaled icon
for a file, directory, or folder as it would be displayed
in a system file browser"</span><o:p></o:p></li>
</ul>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span
class="new"> getSystemIcon(File f, int width,
int height)</span><br>
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.<br>
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.<o:p></o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><i><span
style="color:#1F497D">[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.</span></i></b><span
style="color:#1F497D"><o:p></o:p></span></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l2
level1 lfo2">As per fix of <a
href="https://bugs.openjdk.java.net/browse/JDK-8151385"
moz-do-not-send="true"><i>JDK-8151385</i></a>, <i>MAX_ICON_SIZE=128
</i>was added so even if you accept width/height as 256 (by
your check it will still restrict icon size to 128)...<o:p></o:p></li>
</ul>
<pre><span class="new"> 287 if((width > 256 || width < 1) || (height > 256 || height < 1)) {</span><o:p></o:p></pre>
<pre><span class="new"> 288 System.err.println("Warning: Icon scaling may be distorted");</span><o:p></o:p></pre>
<pre><span class="new"> 289 throw new IllegalArgumentException("unexpected icon scaling size");</span><o:p></o:p></pre>
<pre><span class="new"> 290 }</span><o:p></o:p></pre>
<p class="MsoNormal"> so maybe either increase
MAX_ICON_SIZE or adjust your check accordingly. <o:p></o:p></p>
<p class="MsoNormal"><b><i><span style="color:#1F497D">[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.</span></i></b><span
style="color:#1F497D"><o:p></o:p></span></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l1
level1 lfo3">We normally do not throw unchecked exception
(RuntimeException) from public API, so you should consider
throwing checked exception instead.<o:p></o:p></li>
</ul>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[Shashi]
Fixed this.</span></i></b><span style="color:#1F497D"><o:p></o:p></span></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l0
level1 lfo4">Also, you need to change javadoc to have
@throws instead of @exception<o:p></o:p></li>
</ul>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[Shashi]
Fixed this.</span></i></b><span style="color:#1F497D"><o:p></o:p></span></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l4
level1 lfo5">Also, it was asked, IIUC that the public API
used MutliResolutionImage and uses <a
href="https://docs.oracle.com/javase/10/docs/api/java/awt/image/MultiResolutionImage.html#getResolutionVariants%28%29"
moz-do-not-send="true"><span
style="font-size:10.0pt;font-family:"Courier
New"">getResolutionVariants</span></a><code><span
style="font-size:10.0pt">() to select appropriate icon
image or let user select it (for case mentioned above,
if we are not sure ourselves)</span></code><code><span
style="font-size:10.0pt;font-family:"Calibri",sans-serif"><o:p></o:p></span></code></li>
</ul>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[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.</span></i></b><span
style="color:#1F497D"><o:p></o:p></span></p>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span class="new"><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span></span><!--[endif]--><span class="new">scaleIconImage, scaleIcon uses lot of common code so we can have a common method<o:p></o:p></span></pre>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[Shashi]
Fixed this.</span></i></b><span style="color:#1F497D"><o:p></o:p></span></p>
<ul type="disc">
<li class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;mso-list:l4
level1 lfo5"><span class="changed">makeIcon(long hIcon, int
bsize)</span>.. I guess parameter name bsize does not
convey meaning, is it "bestsize"?<o:p></o:p></li>
</ul>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[Shashi]
Just an alternative and no meaning for that.</span></i></b><span
style="color:#1F497D"><o:p></o:p></span></p>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]--><span class="new">int size = getLargeIcon ? 32 : 16; and </span><span class="changed">also in Win32ShellFolderManager2. java </span><span class="new">probably you can have constants for these 2 number</span><o:p></o:p></pre>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span class="changed"><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span></span><!--[endif]--><span class="changed">Win32ShellFolderManager2<o:p></o:p></span></pre>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]--><span class="changed">1118 return getShell32Icon(4, size);</span><o:p></o:p></pre>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]-->1119 } else {<o:p></o:p></pre>
<pre style="margin-left:.5in;text-indent:-.25in;mso-list:l4 level1 lfo5"><!--[if !supportLists]--><span style="font-family:Symbol"><span style="mso-list:Ignore">·<span style="font:7.0pt "Times New Roman""> </span></span></span><!--[endif]--><span class="changed">1120 return getShell32Icon(1, size);</span><o:p></o:p></pre>
<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:.5in"><span
class="new">You should use constant to be more readable
FILE_ICON_ID = 1; FOLDER_ICON_ID = 4;<o:p></o:p></span></p>
<p class="MsoListParagraph"><b><i><span style="color:#1F497D">[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!!</span></i></b><span
style="color:#1F497D"><o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt">Regards<br>
Prasanta<o:p></o:p></p>
<div>
<p class="MsoNormal">On 7/20/2018 12:02 PM, Shashidhara
Veerabhadraiah wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal">Hi All, Please review a fix for the below
bug.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8182043"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8182043</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Webrev: <a
href="http://cr.openjdk.java.net/%7Esveerabhadra/8182043/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sveerabhadra/8182043/webrev.00/</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">History: This fix is derived from an
earlier fix proposed under this mail thread: <a
href="http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013016.html"
moz-do-not-send="true">http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013016.html</a>.
Thanks to Semyon for that trial and part of this solution is
continued from where it was left off.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">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.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Thanks and regards,<o:p></o:p></p>
<p class="MsoNormal">Shashi<o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>