<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    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>
    The result of the call is ignored now. Is it intentional?<br>
    <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>
    <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>
    <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">http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/</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>
  </body>
</html>