<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Alexey,<br>
    <br>
    On 9/26/2017 12:29 PM, Alexey Ivanov wrote:<br>
    <blockquote
      cite="mid:9331b1e4-dea0-5f2e-3afd-a9e4a8010fc3@oracle.com"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
      Hi Semyon,<br>
      <br>
      ShellFolder2.cpp<br>
      <pre><span class="changed">944             pIcon->Extract(szBuf, index, &hIcon, <b>0</b>, sz);</span></pre>
      I think 0 should really be <code>NULL</code>.<br>
    </blockquote>
    Ok, but both represent the null pointer in Win32.<br>
    <br>
    <blockquote
      cite="mid:9331b1e4-dea0-5f2e-3afd-a9e4a8010fc3@oracle.com"
      type="cite"> The result of the call is ignored now. Is it
      intentional?<br>
    </blockquote>
    Yes, it has been working the same way before the fix. The function
    returns the Icon reference which is 0 in case of OS API call error.<br>
    <br>
    <blockquote
      cite="mid:9331b1e4-dea0-5f2e-3afd-a9e4a8010fc3@oracle.com"
      type="cite"> <br>
      Win32ShellFolder2.java<br>
      <pre><span class="changed"><span class="changed">1010     private static Image makeIcon(long hIcon, int bsize) {
</span></span></pre>
      <code>bsize</code> was called <code>baseSize</code> previously,
      and it conveyed the meaning better, didn't it?<br>
      <br>
      <pre><span class="new">1043                 if(hIcon <= 0) {</span>
<span class="new">1044                     if (isDirectory()) {</span>
<span class="new">1045                         return getShell32Icon(4, size);</span>
<span class="new">1046                     } else {</span>
<span class="new">1047                         return getShell32Icon(1, size);</span>
<span class="new">1048                     }</span></pre>
      I guess I understand what hides behind 4 and 1: generic folder and
      generic file icon ids. Would declaring these numbers as constants
      improve readability?<br>
    </blockquote>
    Ok.<br>
    <blockquote
      cite="mid:9331b1e4-dea0-5f2e-3afd-a9e4a8010fc3@oracle.com"
      type="cite"> <br>
      <code>getIcon(int size)</code> and <code>getIcon(boolean
        getLargeIcon)</code> are somewhat similar. The latter provides
      additional caching. Can it be simplified to re-use portions of new
      <code>getIcon(int size)</code>?<br>
    </blockquote>
    It has additional difference because of query for a fixed icon size
    from another API call.  It's better to leave it as it is.<br>
    <br>
    <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/</a><br>
    <br>
    --Semyon<br>
    <blockquote
      cite="mid:9331b1e4-dea0-5f2e-3afd-a9e4a8010fc3@oracle.com"
      type="cite"> <br>
      <br>
      Regards,<br>
      Alexey<br>
      <br>
      <div class="moz-cite-prefix">On 22/09/2017 22:05, Semyon Sadetsky
        wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:125d7526-1b85-d950-41c6-fbadebbde114@oracle.com">
        <meta http-equiv="Content-Type" content="text/html;
          charset=utf-8">
        <p>Hi Alexey,<br>
        </p>
        <div class="moz-cite-prefix">On 09/22/2017 01:01 PM, Alexey
          Ivanov wrote:<br>
        </div>
        <blockquote type="cite"
          cite="mid:d0f15f7b-70d1-4f44-5984-ea073e8535d9@oracle.com">
          <meta http-equiv="Content-Type" content="text/html;
            charset=utf-8">
          Hi Semyon,<br>
          <br>
          <div class="moz-cite-prefix">On 22/09/2017 20:06, Semyon
            Sadetsky wrote:<br>
          </div>
          <blockquote type="cite"
            cite="mid:904ab740-1ac3-cf14-280e-bfa78e9d908d@oracle.com">
            <meta http-equiv="Content-Type" content="text/html;
              charset=utf-8">
            <p>Hi Alexey, </p>
            <div class="moz-cite-prefix">On 09/22/2017 10:53 AM, Alexey
              Ivanov wrote:<br>
            </div>
            <blockquote type="cite"
              cite="mid:bdc6eba7-92c1-b5c3-d2bf-4557487f50e5@oracle.com">Hi

              Semyon, <br>
              <br>
              On 22/09/2017 18:29, Semyon Sadetsky wrote: <br>
              <blockquote type="cite">Hi Alexey, <br>
                <br>
                On 09/22/2017 09:43 AM, Alexey Ivanov wrote: <br>
                <blockquote type="cite">Hi Semyon, <br>
                  <br>
                  On 22/09/2017 17:13, Semyon Sadetsky wrote: <br>
                  <blockquote type="cite">Hi Alexey, <br>
                    <br>
                    Thank you for your exact clarification. <br>
                    <br>
                    On 09/22/2017 04:22 AM, Alexey Ivanov wrote: <br>
                    <blockquote type="cite"><SNIP> <br>
                      <br>
                      As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd
                      suggest using Windows API to retrieve the
                      recommended size for small and large icon size
                      rather than defaulting to 16×16 and 32×32. If
                      HiDPI is in effect, the icons must be larger. <br>
                    </blockquote>
                    I also found this as most suitable approach for the
                    moment. <br>
                    Later this may be changed, for example, if Swing JFC
                    is re-factored to support shell determined icon
                    sizes at HiDPI. <br>
                  </blockquote>
                  <br>
                  Swing UI scales to accommodate HiDPI settings. If
                  fonts are larger then icons should be larger too.
                  Otherwise icons are too small compared to surrounding
                  text. <br>
                  <br>
                  Anyway it could be postponed to a later fix. <br>
                  <br>
                  Does it make sense to declare the standard sizes of
                  16×16 and 32×32 as constants at least in Java sources?
                  This way, it would be easier to find the places in
                  code where a change is necessary. <br>
                </blockquote>
                This topic requires more investigations. At first, we
                need to keep the API cross-platform and this requires
                comparing all supported platforms in details. At the
                second, even for the existing windows implementation
                there is an ambiguity in icons sizes received form the
                OS shell. Windows platform has number of predefined
                constants to query icon sizes (small, large, extra
                large, jumbo...) but their actual size may differ
                depending on the shell preferences. <br>
                Those icon sizes may be changed by Windows registry
                settings and may depend on the hi-res scale. I did
                several experiments and found that depending on the way
                of desktop scaling  in Windows 10 (it has two ways the
                new one and the old) at the same scale 2x the returned
                large icon, for example,  may be 32 or 64 pixels in size
                (this was the root cause of the 8151385  bug). <br>
                I would postpone digging in this direction because we
                are not planning to update Swing JFC dialog for better
                HiDPI view in the nearest future. Also,we don't have
                statistics how users may use the API. Since that, the
                most flexible API that leaves to the user the decision
                about icon size to query seems more preferable. <br>
              </blockquote>
              <br>
              I totally agree with your points. And the new API provides
              means for app developer to choose better-suited size for
              icons. <br>
              <br>
              What about using constants, private ones, for the two
              standard sizes instead of using “magic” numbers? <br>
            </blockquote>
            Which constants do you mean? The next for large and small
            sizes?<br>
            <br>
            p<span style="color:#000080;font-weight:bold;">ublic static
              final int </span><span
              style="color:#503b5f;font-style:italic;">FILE_ICON_SMALL </span><span
              style="font-weight:bold;">= -</span><span
              style="color:#0000ff;">1</span>;<span
              style="color:#2f354d;font-style:italic;"></span><br>
            <span style="color:#2f354d;font-style:italic;"></span>
            <pre style="background-color:#ffffff;color:#000000;font-family:'Source Code Pro';font-size:10.5pt;"><span style="color:#000080;font-weight:bold;">public static final int </span><span style="color:#503b5f;font-style:italic;">FILE_ICON_LARGE </span><span style="font-weight:bold;">= -</span><span style="color:#0000ff;">2</span>;<span style="color:#2f354d;font-style:italic;"></span></pre>
            they cannot be private because they are part of the new API
            that is requested in the bug. The bug asks enabling the
            query for the large icon that ShellFolder had been providing
            before. The bug itself is not related to HiDPI. The
            possibility to get arbitrary icon sizes was added because
            after 9 this may be in demand for HiDPI UIs.<br>
          </blockquote>
          <br>
          I'm talking about these two numbers in Win32ShellFolder2.java
          as an example:<br>
          <br>
             public Image getIcon(final boolean getLargeIcon) {<br>
                 int size = getLargeIcon ? <b>32</b> : <b>16</b>;<br>
          <br>
          Does it make sense to declare constants for the size of 16 and
          32. So that the places where they're used are more easily
          identified if someone decides to update the code later for
          supporting system icon size.<br>
        </blockquote>
        I updated the fix.<br>
        <br>
        <a class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.01/"
          moz-do-not-send="true">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/</a><br>
        <br>
        --Semyon<br>
        <blockquote type="cite"
          cite="mid:d0f15f7b-70d1-4f44-5984-ea073e8535d9@oracle.com"> <br>
          <br>
          Regards,<br>
          Alexey<br>
          <br>
          <blockquote type="cite"
            cite="mid:904ab740-1ac3-cf14-280e-bfa78e9d908d@oracle.com">
            <br>
            --Semyon<br>
            <br>
            <blockquote type="cite"
              cite="mid:bdc6eba7-92c1-b5c3-d2bf-4557487f50e5@oracle.com">Other

              than that, the fix looks good to me. <br>
              <br>
              <br>
              Regards, <br>
              Alexey <br>
              <br>
              <blockquote type="cite"> <br>
                <br>
                --Semyon <br>
                <blockquote type="cite"> <br>
                  <br>
                  Regards, <br>
                  Alexey <br>
                  <br>
                  <blockquote type="cite"> <br>
                    <br>
                    --Semyon <br>
                    <blockquote type="cite"> <br>
                      <br>
                      Regards, <br>
                      Alexey <br>
                      <br>
                      <blockquote type="cite"> <br>
                        <blockquote type="cite"><SNIP> <br>
                          <blockquote type="cite"> <br>
                            <blockquote type="cite">
                              <blockquote type="cite">On 9/13/17 11:01,
                                Semyon Sadetsky wrote: <br>
                                <blockquote type="cite">Hello, <br>
                                  <br>
                                  Please review fix for JDK10 (the
                                  changes involve AWT and Swing): <br>
                                  <br>
                                  bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8182043"
                                    moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8182043</a>
                                  <br>
                                  <br>
                                  webrev: <a
                                    class="moz-txt-link-freetext"
                                    href="http://cr.openjdk.java.net/%7Essadetsky/8182043/webrev.00/"
                                    moz-do-not-send="true"><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/</a></a>
                                  <br>
                                  <br>
                                  The fix  opens the part of the
                                  ShellFolder API for getting system
                                  icons which was decided to be left
                                  closed during the 8081722 enhancement
                                  review in 9. <br>
                                  <br>
                                  Also the fix extends the API by adding
                                  possibility to query file icons of
                                  arbitrary size and implements this
                                  extension for Windows platform. <br>
                                  <br>
                                  --Semyon <br>
                                  <br>
                                </blockquote>
                                <br>
                                <br>
                              </blockquote>
                              <br>
                            </blockquote>
                            <br>
                            <br>
                          </blockquote>
                          <br>
                        </blockquote>
                        <br>
                        <br>
                      </blockquote>
                      <br>
                    </blockquote>
                    <br>
                  </blockquote>
                  <br>
                </blockquote>
                <br>
              </blockquote>
              <br>
            </blockquote>
            <br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>