<AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Alexey Ushakov-3
Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

-------------

Commit messages:
 - 8258788: incorrect response to change in window insets [lanai]

Changes: https://git.openjdk.java.net/jdk/pull/3308/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8258788
  Stats: 31 lines in 7 files changed: 16 ins; 13 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3308.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3308/head:pull/3308

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Sergey Bylokhov-2
On Thu, 1 Apr 2021 14:38:52 GMT, Alexey Ushakov <[hidden email]> wrote:

> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

Is it possible to automatically test it?

src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m line 1171:

> 1169:                         layer.leftInset = (jint)(screenContentRect.origin.x - frame.origin.x);
> 1170:                     }
> 1171:                 }

Can you check that it will work in the "new" tabbed mode on big sur as well? Probably it is possible to swap coordinates during rendering in the layer and get rid of this field?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v2]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

Alexey Ushakov has updated the pull request incrementally with one additional commit since the last revision:

  8258788: incorrect response to change in window insets [lanai]
 
  Added regression test

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3308/files
  - new: https://git.openjdk.java.net/jdk/pull/3308/files/afab7f43..cfe15982

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=00-01

  Stats: 165 lines in 1 file changed: 165 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3308.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3308/head:pull/3308

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Alexey Ushakov-3
In reply to this post by Sergey Bylokhov-2
On Thu, 1 Apr 2021 15:15:32 GMT, Sergey Bylokhov <[hidden email]> wrote:

> Is it possible to automatically test it?

Yes, just added the test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v2]

Kevin Rushforth-2
In reply to this post by Alexey Ushakov-3
On Fri, 2 Apr 2021 11:44:52 GMT, Alexey Ushakov <[hidden email]> wrote:

>> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.
>
> Alexey Ushakov has updated the pull request incrementally with one additional commit since the last revision:
>
>   8258788: incorrect response to change in window insets [lanai]
>  
>   Added regression test

test/jdk/java/awt/Window/FullWindowContentTest/FullWindowContentRenderTest.java line 9:

> 7:  * published by the Free Software Foundation.  Oracle designates this
> 8:  * particular file as subject to the "Classpath" exception as provided
> 9:  * by Oracle in the LICENSE file that accompanied this code.

Tests should not have the "Classpath" exception.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v3]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

Alexey Ushakov has updated the pull request incrementally with one additional commit since the last revision:

  8258788: incorrect response to change in window insets [lanai]
 
  Corrected copyrights

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3308/files
  - new: https://git.openjdk.java.net/jdk/pull/3308/files/cfe15982..52fc75e7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=01-02

  Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3308.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3308/head:pull/3308

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v3]

Alexey Ushakov-3
In reply to this post by Sergey Bylokhov-2
On Thu, 1 Apr 2021 15:18:21 GMT, Sergey Bylokhov <[hidden email]> wrote:

> Can you check that it will work in the "new" tabbed mode on big sur as well?

Could you suggest any working test scenario? My simple test (below) just hangs even with OGL:

import java.awt.FlowLayout;
import java.util.Objects;
import javax.swing.*;
import javax.swing.border.EmptyBorder;

public class TestWindowInsets
  extends JDialog
{

    public TestWindowInsets()
    {
        JComponent contentPane = (JComponent) getContentPane();
        contentPane.setBorder(new EmptyBorder(50, 50, 50, 50));
        JButton b = new JButton("Test");
        b.addActionListener(e -> toggle());
        add(b);
        JButton c = new JButton("Win");
        c.addActionListener(e -> win());
        add(c);

        setLayout(new FlowLayout());
        setSize(800, 600);

        setVisible(true);

    }

    void toggle()
    {
        SwingUtilities.invokeLater(() -> {
            JRootPane rp = getRootPane();
            String name = "apple.awt.fullWindowContent";
            Object value = rp.getClientProperty(name);
            if (Objects.equals(value, "true")) {
                value = "false";
            } else {
                value = "true";
            }
            rp.putClientProperty(name, value);
        });
    }

    void win()
    {
        SwingUtilities.invokeLater(TestWindowInsets::new);
    }

    public static void main(String[] args)
    {
        SwingUtilities.invokeLater(TestWindowInsets::new);
    }
}

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v3]

Alexey Ushakov-3
On Mon, 5 Apr 2021 10:29:20 GMT, Alexey Ushakov <[hidden email]> wrote:

>> src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m line 1171:
>>
>>> 1169:                         layer.leftInset = (jint)(screenContentRect.origin.x - frame.origin.x);
>>> 1170:                     }
>>> 1171:                 }
>>
>> Can you check that it will work in the "new" tabbed mode on big sur as well? Probably it is possible to swap coordinates during rendering in the layer and get rid of this field?
>
>> Can you check that it will work in the "new" tabbed mode on big sur as well?
>
> Could you suggest any working test scenario? My simple test (below) just hangs even with OGL:
>
> import java.awt.FlowLayout;
> import java.util.Objects;
> import javax.swing.*;
> import javax.swing.border.EmptyBorder;
>
> public class TestWindowInsets
>   extends JDialog
> {
>
>     public TestWindowInsets()
>     {
>         JComponent contentPane = (JComponent) getContentPane();
>         contentPane.setBorder(new EmptyBorder(50, 50, 50, 50));
>         JButton b = new JButton("Test");
>         b.addActionListener(e -> toggle());
>         add(b);
>         JButton c = new JButton("Win");
>         c.addActionListener(e -> win());
>         add(c);
>
>         setLayout(new FlowLayout());
>         setSize(800, 600);
>
>         setVisible(true);
>
>     }
>
>     void toggle()
>     {
>         SwingUtilities.invokeLater(() -> {
>             JRootPane rp = getRootPane();
>             String name = "apple.awt.fullWindowContent";
>             Object value = rp.getClientProperty(name);
>             if (Objects.equals(value, "true")) {
>                 value = "false";
>             } else {
>                 value = "true";
>             }
>             rp.putClientProperty(name, value);
>         });
>     }
>
>     void win()
>     {
>         SwingUtilities.invokeLater(TestWindowInsets::new);
>     }
>
>     public static void main(String[] args)
>     {
>         SwingUtilities.invokeLater(TestWindowInsets::new);
>     }
> }

> Probably it is possible to swap coordinates during rendering in the layer and get rid of this field?

I don't see a way how we can do it. Because of the inverted y coordinate in metal, we need to place the origin of the drawing with the appropriate offset that depends on the title height, even if we invert the y coordinate to match java2d coordinate system.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Jayathirth D V-2
In reply to this post by Alexey Ushakov-3
On Fri, 2 Apr 2021 11:41:12 GMT, Alexey Ushakov <[hidden email]> wrote:

> > Is it possible to automatically test it?
>
> Yes, just added the test.

@avu Test passes without fix also.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Jayathirth D V-2
On Tue, 6 Apr 2021 12:03:14 GMT, Jayathirth D V <[hidden email]> wrote:

>>> Is it possible to automatically test it?
>>
>> Yes, just added the test.
>
>> > Is it possible to automatically test it?
>>
>> Yes, just added the test.
>
> @avu Test passes without fix also.

Verified test case attached in JBS : https://bugs.openjdk.java.net/browse/JDK-8258788 . I see that fix resolves identified issue in JBS. Also jtreg and JCK test run is green with and without Metal API validation flags.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
On Fri, 2 Apr 2021 11:41:12 GMT, Alexey Ushakov <[hidden email]> wrote:

>> Is it possible to automatically test it?
>
>> Is it possible to automatically test it?
>
> Yes, just added the test.

> @avu Test passes without fix also.
@jayathirthrao Could you provide the details about your configuration along with parameters passed to jtreg ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Jayathirth D V-2
In reply to this post by Jayathirth D V-2
On Tue, 6 Apr 2021 12:03:14 GMT, Jayathirth D V <[hidden email]> wrote:

> > > Is it possible to automatically test it?
> >
> >
> > Yes, just added the test.
>
> @avu Test passes without fix also.

@aghaisas Please verify the regression test behavior from your end also.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v3]

Sergey Bylokhov-2
In reply to this post by Alexey Ushakov-3
On Mon, 5 Apr 2021 10:34:21 GMT, Alexey Ushakov <[hidden email]> wrote:

>>> Can you check that it will work in the "new" tabbed mode on big sur as well?
>>
>> Could you suggest any working test scenario? My simple test (below) just hangs even with OGL:
>>
>> import java.awt.FlowLayout;
>> import java.util.Objects;
>> import javax.swing.*;
>> import javax.swing.border.EmptyBorder;
>>
>> public class TestWindowInsets
>>   extends JDialog
>> {
>>
>>     public TestWindowInsets()
>>     {
>>         JComponent contentPane = (JComponent) getContentPane();
>>         contentPane.setBorder(new EmptyBorder(50, 50, 50, 50));
>>         JButton b = new JButton("Test");
>>         b.addActionListener(e -> toggle());
>>         add(b);
>>         JButton c = new JButton("Win");
>>         c.addActionListener(e -> win());
>>         add(c);
>>
>>         setLayout(new FlowLayout());
>>         setSize(800, 600);
>>
>>         setVisible(true);
>>
>>     }
>>
>>     void toggle()
>>     {
>>         SwingUtilities.invokeLater(() -> {
>>             JRootPane rp = getRootPane();
>>             String name = "apple.awt.fullWindowContent";
>>             Object value = rp.getClientProperty(name);
>>             if (Objects.equals(value, "true")) {
>>                 value = "false";
>>             } else {
>>                 value = "true";
>>             }
>>             rp.putClientProperty(name, value);
>>         });
>>     }
>>
>>     void win()
>>     {
>>         SwingUtilities.invokeLater(TestWindowInsets::new);
>>     }
>>
>>     public static void main(String[] args)
>>     {
>>         SwingUtilities.invokeLater(TestWindowInsets::new);
>>     }
>> }
>
>> Probably it is possible to swap coordinates during rendering in the layer and get rid of this field?
>
> I don't see a way how we can do it. Because of the inverted y coordinate in metal, we need to place the origin of the drawing with the appropriate offset that depends on the title height, even if we invert the y coordinate to match java2d coordinate system.

I am just not sure that this is the only place where the insets might be changed.

We need to add a "callback" which will be called every time the insets will be changed. Probably in the MTLLayer.replaceSurfaceData which should be called from the LWWindowsPeer#notifyReshape->replaceSurfaceData(). Currently, we skip that call if insets were changed. and this is why this bug arise.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai] [v4]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

Alexey Ushakov has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:

  8258788: incorrect response to change in window insets [lanai]
 
  Perform replaceSurfaceData on insets change

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3308/files
  - new: https://git.openjdk.java.net/jdk/pull/3308/files/52fc75e7..ed5e6814

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3308&range=02-03

  Stats: 35 lines in 8 files changed: 17 ins; 17 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3308.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3308/head:pull/3308

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> Withdrawn: 8258788: incorrect response to change in window insets [lanai]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
On Thu, 1 Apr 2021 14:38:52 GMT, Alexey Ushakov <[hidden email]> wrote:

> Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup.

This pull request has been closed without being integrated.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8258788: incorrect response to change in window insets [lanai]

Alexey Ushakov-3
In reply to this post by Alexey Ushakov-3
On Wed, 7 Apr 2021 20:29:12 GMT, Alexey Ushakov <[hidden email]> wrote:

>>> @avu Test passes without fix also.
>> @jayathirthrao Could you provide the details about your configuration along with parameters passed to jtreg ?
>
>> @avu I am running test in 13 inch Macbook Early 2015 with integrated Intel Iris Graphics 6100.
>> jtreg command i am using "jtreg -jdk:<path_to_jdk> -Dsun.java2d.metal=true <path_to_test>"
> @jayathirthrao  I'm not sure that -Dsun.java2d.metal had been passed to the test JVM. I rechecked with slightly different command line and the test fails without the fix
> jtreg -ignore:quiet -v -a -xml -vmoptions:"-Dsun.java2d.metal=True "  -testjdk:path_to_jdk path_to_test

I've created a simpler solution within another pull request. Please, have a look https://git.openjdk.java.net/jdk/pull/3390

-------------

PR: https://git.openjdk.java.net/jdk/pull/3308