<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    +1 from me for the JDK changes.<br>
    <br>
    -phil.<br>
    <br>
    <div class="moz-cite-prefix">On 06/13/2018 06:04 PM, Kevin Rushforth
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:d2ca2e89-215d-34b1-73f8-064c40eb512d@oracle.com">
      <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
      The JDK changes look good to me, and as far as I have tested them,
      it works for me. I haven't tried Drag and Drop yet with the latest
      interface changes.<br>
      <br>
      To repeat something I've mentioned before, just so there is no
      confusion, the FX side of the changes are intended to be a proof
      of concept to test the JDK side at this point. They will need
      refactoring before they can go in, so that FX will still be
      buildable and runnable with JDK 10.<br>
      <br>
      -- Kevin<br>
      <br>
      <br>
      <div class="moz-cite-prefix">On 6/13/2018 12:48 AM, Prasanta
        Sadhukhan wrote:<br>
      </div>
      <blockquote type="cite"
        cite="mid:502b95f6-6fc4-39ce-3ca5-b4ec5aab02aa@oracle.com">
        <meta http-equiv="Content-Type" content="text/html;
          charset=utf-8">
        <p>Hi Phil, All<br>
        </p>
        Please find modified webrev taking care of streamlining
        SwingInteropUtils class and handling of native window handle in
        LightweightFrameWrapper class by using JNI call in FX<br>
        <a class="moz-txt-link-freetext"
          href="http://cr.openjdk.java.net/%7Epsadhukhan/fxswing.14/"
          moz-do-not-send="true">http://cr.openjdk.java.net/~psadhukhan/fxswing.14/</a><br>
        <br>
        Corresponding CSR: <a class="moz-txt-link-freetext"
          href="https://bugs.openjdk.java.net/browse/JDK-8202175"
          moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8202175</a><br>
        <br>
        Regards<br>
        Prasanta<br>
        <div class="moz-cite-prefix">On 5/30/2018 4:13 AM, Philip Race
          wrote:<br>
        </div>
        <blockquote type="cite" cite="mid:5B0DD789.8000809@oracle.com">
          <meta content="text/html; charset=utf-8"
            http-equiv="Content-Type">
          Some follow up comments.<br>
          <br>
          On 5/10/18, 5:49 PM, Philip Race wrote:
          <blockquote cite="mid:5AF4E8AF.60202@oracle.com" type="cite">
            <meta content="text/html; charset=utf-8"
              http-equiv="Content-Type">
            I've looked over the Swing code. No time to actually try it
            out so I can only comment on what I see.<br>
            <br>
            I am not sure what I think about class docs like "This class
            wraps <names internal interface> .."<br>
            I know no javadoc is generated but this module is exported
            and probably should say something<br>
            less specific.<br>
            <br>
            In general I really have to hold my nose when looking at
            this whole module and I<br>
            find it distasteful to endorse it.<br>
            <br>
            I really don't like all the things SwingInterOpUtils.java
            does.<br>
            <br>
          </blockquote>
          Specific things we should look at in this file<br>
          - getDefaultScaleX/Y .. JDK 9 will set a transform on the
          graphics that<br>
          is passed to JComponent.paintComponent() .. so I wonder if we
          really<br>
          need this internal API. I doubt any other code is updating the
          transform<br>
          in a way that would make it a problem so the information you
          get should be valid. <br>
          <br>
          The code to get the data array for the raster and associated
          information<br>
          can be extracted via standard API like this :<br>
          <br>
          DataBuffer  db = BufferedImage.getRaster().getDataBuffer();<br>
          if (db instanceof DataBufferInt) {<br>
           data = (DataBufferInt)db.getData();<br>
          }<br>
          <br>
          So all of those can be fixed.<br>
          <br>
          The code that accepts a native window handle maybe should
          require it is accessed via JNI ... <br>
          It could still be unsupported, but it would be package private
          or similar.<br>
          If you have a native ID, then you are already using native
          code.<br>
          This sidesteps any questions about the stability of such an
          API which<br>
          accepts a native resource.<br>
          <br>
          The code that finds a component by location is probably
          harmless ...<br>
          <br>
          I'm less sure about the event posting and focus grabbing
          support.<br>
          I'd like to have Sergey comment on those.<br>
          <br>
          the DnD code is too much for me to examine in detail.<br>
          <blockquote cite="mid:5AF4E8AF.60202@oracle.com" type="cite">
            I worry we are creating something we'll come to regret here.<br>
            The "unsupportedness" needs to be backed up by
            deprecated-for-removal being included<br>
            today so we can get rid of it the next release if we want
            to.<br>
          </blockquote>
          <br>
          I've looked at jdk.unsupported and they don't seem to have
          deprecation tags so<br>
          maybe that isn't an issue.<br>
          <br>
          -phil<br>
          <blockquote cite="mid:5AF4E8AF.60202@oracle.com" type="cite">
            <br>
            No @since tags anywhere....<br>
            <br>
            -phil.<br>
            <br>
            On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
            <blockquote
              cite="mid:fe5995d5-d8ed-6492-a1f2-3d242424b06f@oracle.com"
              type="cite">
              <meta http-equiv="Content-Type" content="text/html;
                charset=utf-8">
              I confirm that this fixes the issue. The interface change
              to make the two static methods e instance methods is a
              good change in any case.<br>
              <br>
              I am not a Swing "R"eviewer, but the .13 webrev gets a +1
              from me.<br>
              <br>
              -- Kevin<br>
              <br>
              <br>
              <div class="moz-cite-prefix">On 5/10/2018 8:20 AM,
                Prasanta Sadhukhan wrote:<br>
              </div>
              <blockquote type="cite"
                cite="mid:bc3b6dd5-6ae2-acaa-81f0-7eef168f271d@oracle.com">
                <meta http-equiv="Content-Type" content="text/html;
                  charset=utf-8">
                <p>Hi Kevin,All,<br>
                </p>
                Please find the modified webrev fixing this #1 issue<br>
                <a class="moz-txt-link-freetext"
                  href="http://cr.openjdk.java.net/%7Epsadhukhan/fxswing.13/"
                  moz-do-not-send="true">http://cr.openjdk.java.net/~psadhukhan/fxswing.13/</a><br>
                via change in
                jdk/swing/interop/DropTargetContextWrapper.java#setDropTargetContext
                and FXDnD.java#postDropTargetEvent<br>
                <br>
                For me, #2 works, #3 doesn't work even now due to <a
                  class="issue-link" data-issue-key="JDK-8141391"
                  href="https://bugs.openjdk.java.net/browse/JDK-8141391"
                  id="key-val" rel="4851839" moz-do-not-send="true">JDK-8141391</a>
                and #4 works for me.<br>
                <br>
                Regards<br>
                Prasanta<br>
                <div class="moz-cite-prefix">On 5/9/2018 11:29 PM, Kevin
                  Rushforth wrote:<br>
                </div>
                <blockquote type="cite"
                  cite="mid:5b57623c-bd12-89af-17e0-1a740d72031c@oracle.com">
                  <meta http-equiv="Content-Type" content="text/html;
                    charset=utf-8">
                  Hi Prasanta,<br>
                  <br>
                  The API looks good now.<br>
                  <br>
                  All of our automated tests work (except for the ones
                  with a security manager due to <a
                    moz-do-not-send="true"
                    href="https://bugs.openjdk.java.net/browse/JDK-8202451">JDK-8202451</a>).<br>
                  <br>
                  The only functional problem that I see is that Drag
                  and Drop onto a SwingNode doesn't work. We need to
                  make sure that we test the following four cases:<br>
                  <br>
                  1. Drag / drop onto a Swing component in a SwingNode<br>
                  2. Drag / drop from a Swing component in a SwingNode<br>
                  3. Drag / drop onto a JavaFX control in a JFXPanel<br>
                  4. Drag / drop from a JavaFX control in a JFXPanel<br>
                  <br>
                  So far I only tried the first one; the others still
                  need to be validated.<br>
                  <br>
                  -- Kevin<br>
                  <br>
                  <br>
                  <br>
                  <div class="moz-cite-prefix">On 5/9/2018 7:14 AM,
                    Prasanta Sadhukhan wrote:<br>
                  </div>
                  <blockquote type="cite"
                    cite="mid:1b7653d1-9c9a-7ad8-8f56-674760136d22@oracle.com">Modified
                    webrev to cater to this <br>
                    <br>
                    <a class="moz-txt-link-freetext"
                      href="http://cr.openjdk.java.net/%7Epsadhukhan/fxswing.12/"
                      moz-do-not-send="true">http://cr.openjdk.java.net/~psadhukhan/fxswing.12/</a>
                    <br>
                    <br>
                    Regards <br>
                    Prasanta <br>
                    On 5/9/2018 5:58 PM, Kevin Rushforth wrote: <br>
                    <blockquote type="cite">The following can also be
                      abstract: <br>
                      <br>
                      LightweightContentWrapper: <br>
                        getComponent, createDragGestureRecognizer,
                      createDragSourceContextPeer <br>
                      <br>
                      DropTargetContextWrapper: <br>
                        getTargetActions, getDropTarget,
                      getTransferDataFlavors, getTransferable,
                      isTransferableJVMLocal <br>
                      <br>
                      DispatcherWrapper: <br>
                        isDispatchThread, createSecondaryLoop <br>
                      <br>
                      The rest looks good to me (although I still see
                      two public methods with "Peer" in the name, so
                      Phil may want those renamed). <br>
                      <br>
                      -- Kevin <br>
                      <br>
                      <br>
                      On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote: <br>
                      <blockquote type="cite">Modified webrev to cater
                        to these 3 observations <br>
                        <a class="moz-txt-link-freetext"
                          href="http://cr.openjdk.java.net/%7Epsadhukhan/fxswing.11/"
                          moz-do-not-send="true">http://cr.openjdk.java.net/~psadhukhan/fxswing.11/</a>
                        <br>
                        <br>
                        Regards <br>
                        Prasanta <br>
                        <br>
                        On 5/9/2018 5:03 AM, Kevin Rushforth wrote: <br>
                        <blockquote type="cite">The module definition
                          for jdk.unsupported.desktop and the changes to
                          java.desktop look fine. <br>
                          <br>
                          In reviewing the jdk.swing.interop API, I have
                          the following suggestions / observations: <br>
                          <br>
                          1. DispatcherWrapper,
                          DragSourceContextWrapper,
                          DropTargetContextWrapper, and
                          LightweightContentWrapper can all be abstract,
                          along with most of the methods (rather than
                          having an empty body return value that is
                          never used). <br>
                          <br>
                          2. The addNotify method in
                          LightweightFrameWrapper is unused. Should be
                          used somewhere? If not, then it can be
                          removed. <br>
                          <br>
                          The implementation of the new wrapper classes
                          looks OK to me with one observation that might
                          or might not matter: <br>
                          <br>
                          3. The behavior of getDefaultScaleX/Y (which
                          is now in SwingInteropUtils) has changed in
                          the case where the Graphics is not an instance
                          of SunGraphics2D. The former behavior was to
                          leave the instance variables X and Y
                          unchanged. The new behavior will set them back
                          to 1.0. Maybe this can't happen in practice,
                          but it is something to consider. <br>
                          <br>
                          -- Kevin <br>
                          <br>
                          <br>
                          On 5/8/2018 3:31 AM, Alan Bateman wrote: <br>
                          <blockquote type="cite">On 08/05/2018 06:51,
                            Prasanta Sadhukhan wrote: <br>
                            <blockquote type="cite">Modified webrev to
                              rename to InteropProviderImpl <br>
                              <br>
                              <a class="moz-txt-link-freetext"
                                href="http://cr.openjdk.java.net/%7Epsadhukhan/fxswing.10/"
                                moz-do-not-send="true">http://cr.openjdk.java.net/~psadhukhan/fxswing.10/</a>
                              <br>
                            </blockquote>
                            This looks okay to me. <br>
                            <br>
                            -Alan <br>
                          </blockquote>
                          <br>
                        </blockquote>
                        <br>
                      </blockquote>
                      <br>
                    </blockquote>
                    <br>
                  </blockquote>
                  <br>
                </blockquote>
                <br>
              </blockquote>
              <br>
            </blockquote>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>