Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274932: Render scales in EmbeddedWindow are not properly updated #1171

Closed
wants to merge 11 commits into from

Conversation

prsadhuk
Copy link
Collaborator

@prsadhuk prsadhuk commented Jul 6, 2023

When the JavaFX scene is set before it is really shown, then the scale factors are not properly propagated to the EmbeddedWindow, resulting in showing wrong scales.
Fix is made to update scales to EmbeddedWindow


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

  • JDK-8274932: Render scales in EmbeddedWindow are not properly updated (Bug - P4)
  • JDK-8222209: JavaFX is rendered blurry on systems with monitors in different configuration (Bug - P4)

Reviewers

Contributors

  • John Hendrikx <jhendrikx@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1171/head:pull/1171
$ git checkout pull/1171

Update a local copy of the PR:
$ git checkout pull/1171
$ git pull https://git.openjdk.org/jfx.git pull/1171/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1171

View PR using the GUI difftool:
$ git pr show -t 1171

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1171.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 6, 2023

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Jul 6, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 6, 2023

@andy-goryachev-oracle
Copy link
Contributor

@prsadhuk could you update to the latest master branch? I am getting build errors...

@andy-goryachev-oracle
Copy link
Contributor

Testing on Mac with 2 monitors (primary scale=2, secondary scale=1), running the Main class from the ticket.

  1. getting exception:
Exception in thread "AWT-EventQueue-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = AWT-EventQueue-0
	at javafx.graphics/com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:293)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:475)
	at javafx.graphics/javafx.scene.Parent$3.onProposedChange(Parent.java:475)
	at javafx.base/com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:116)
	at javafx.base/com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:110)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.updateChildren(LabeledSkinBase.java:282)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.lambda$11(LabeledSkinBase.java:219)
	at javafx.controls/com.sun.javafx.scene.control.LambdaMultiplePropertyChangeListenerHandler.lambda$1(LambdaMultiplePropertyChangeListenerHandler.java:88)
	at javafx.base/javafx.beans.value.WeakChangeListener.changed(WeakChangeListener.java:86)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:192)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:91)
	at javafx.base/javafx.beans.property.StringPropertyBase.fireValueChangedEvent(StringPropertyBase.java:104)
	at javafx.base/javafx.beans.property.StringPropertyBase.markInvalid(StringPropertyBase.java:111)
	at javafx.base/javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:145)
	at javafx.base/javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:1)
	at javafx.base/javafx.beans.property.StringProperty.setValue(StringProperty.java:71)
	at javafx.controls/javafx.scene.control.Labeled.setText(Labeled.java:147)
	at andy_test/goryachev.apps.EmbeddedFrameBug.updateText(EmbeddedFrameBug.java:86)
	at andy_test/goryachev.apps.EmbeddedFrameBug.lambda$3(EmbeddedFrameBug.java:69)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:192)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:91)
	at javafx.base/javafx.beans.property.DoublePropertyBase.fireValueChangedEvent(DoublePropertyBase.java:107)
	at javafx.base/javafx.beans.property.DoublePropertyBase.markInvalid(DoublePropertyBase.java:114)
	at javafx.base/javafx.beans.property.DoublePropertyBase.set(DoublePropertyBase.java:148)
	at javafx.graphics/javafx.stage.Window.setRenderScaleX(Window.java:494)
	at javafx.swing/javafx.embed.swing.JFXPanel.updateComponentSize(JFXPanel.java:640)
	at javafx.swing/javafx.embed.swing.JFXPanel.addNotify(JFXPanel.java:945)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/javax.swing.JRootPane.addNotify(JRootPane.java:721)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/java.awt.Window.addNotify(Window.java:791)
	at java.desktop/java.awt.Frame.addNotify(Frame.java:495)
	at java.desktop/java.awt.Window.show(Window.java:1053)
	at java.desktop/java.awt.Component.show(Component.java:1728)
	at java.desktop/java.awt.Component.setVisible(Component.java:1675)
	at java.desktop/java.awt.Window.setVisible(Window.java:1036)
	at andy_test/goryachev.apps.EmbeddedFrameBug.setFrameVisible(EmbeddedFrameBug.java:51)
	at andy_test/goryachev.apps.EmbeddedFrameBug.lambda$1(EmbeddedFrameBug.java:45)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:318)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:773)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:720)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:714)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:87)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

  1. on primary retina monitor, the UI looks good:
Screenshot 2023-07-07 at 09 18 05

on the secondary, not so good:

Screenshot 2023-07-07 at 09 18 01

@andy-goryachev-oracle
Copy link
Contributor

Appearance without the fix on the secondary is almost identical:

Screenshot 2023-07-07 at 09 23 48

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Jul 7, 2023

Testing on Mac with 2 monitors (primary scale=2, secondary scale=1), running the Main class from the ticket.

  1. getting exception:
Exception in thread "AWT-EventQueue-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = AWT-EventQueue-0
	at javafx.graphics/com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:293)
	at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:475)
	at javafx.graphics/javafx.scene.Parent$3.onProposedChange(Parent.java:475)
	at javafx.base/com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:116)
	at javafx.base/com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.java:110)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.updateChildren(LabeledSkinBase.java:282)
	at javafx.controls/javafx.scene.control.skin.LabeledSkinBase.lambda$11(LabeledSkinBase.java:219)
	at javafx.controls/com.sun.javafx.scene.control.LambdaMultiplePropertyChangeListenerHandler.lambda$1(LambdaMultiplePropertyChangeListenerHandler.java:88)
	at javafx.base/javafx.beans.value.WeakChangeListener.changed(WeakChangeListener.java:86)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:192)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:91)
	at javafx.base/javafx.beans.property.StringPropertyBase.fireValueChangedEvent(StringPropertyBase.java:104)
	at javafx.base/javafx.beans.property.StringPropertyBase.markInvalid(StringPropertyBase.java:111)
	at javafx.base/javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:145)
	at javafx.base/javafx.beans.property.StringPropertyBase.set(StringPropertyBase.java:1)
	at javafx.base/javafx.beans.property.StringProperty.setValue(StringProperty.java:71)
	at javafx.controls/javafx.scene.control.Labeled.setText(Labeled.java:147)
	at andy_test/goryachev.apps.EmbeddedFrameBug.updateText(EmbeddedFrameBug.java:86)
	at andy_test/goryachev.apps.EmbeddedFrameBug.lambda$3(EmbeddedFrameBug.java:69)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:192)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:91)
	at javafx.base/javafx.beans.property.DoublePropertyBase.fireValueChangedEvent(DoublePropertyBase.java:107)
	at javafx.base/javafx.beans.property.DoublePropertyBase.markInvalid(DoublePropertyBase.java:114)
	at javafx.base/javafx.beans.property.DoublePropertyBase.set(DoublePropertyBase.java:148)
	at javafx.graphics/javafx.stage.Window.setRenderScaleX(Window.java:494)
	at javafx.swing/javafx.embed.swing.JFXPanel.updateComponentSize(JFXPanel.java:640)
	at javafx.swing/javafx.embed.swing.JFXPanel.addNotify(JFXPanel.java:945)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/javax.swing.JComponent.addNotify(JComponent.java:4846)
	at java.desktop/javax.swing.JRootPane.addNotify(JRootPane.java:721)
	at java.desktop/java.awt.Container.addNotify(Container.java:2804)
	at java.desktop/java.awt.Window.addNotify(Window.java:791)
	at java.desktop/java.awt.Frame.addNotify(Frame.java:495)
	at java.desktop/java.awt.Window.show(Window.java:1053)
	at java.desktop/java.awt.Component.show(Component.java:1728)
	at java.desktop/java.awt.Component.setVisible(Component.java:1675)
	at java.desktop/java.awt.Window.setVisible(Window.java:1036)
	at andy_test/goryachev.apps.EmbeddedFrameBug.setFrameVisible(EmbeddedFrameBug.java:51)
	at andy_test/goryachev.apps.EmbeddedFrameBug.lambda$1(EmbeddedFrameBug.java:45)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:318)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:773)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:720)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:714)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:400)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:87)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)
  1. on primary retina monitor, the UI looks good:
Screenshot 2023-07-07 at 09 18 05 on the secondary, not so good:

Screenshot 2023-07-07 at 09 18 01

That is tracked on different bugid https://bugs.openjdk.org/browse/JDK-8222209

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Jul 7, 2023

Exception in thread "AWT-EventQueue-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = AWT-EventQueue-0
at javafx.graphics/com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:293)
at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:475)
at javafx.graphics/javafx.scene.Parent$3.onProposedChange(Parent.java:475)
at javafx.base/com.sun.javafx.collections.VetoableListDecorator.setAll(VetoableListDecorator.jav

You need to run with this change

private static void initAndShowGUI() throws Exception
   {
      JFrame frame = new JFrame("Swing and JavaFX");
      final JFXPanel fxPanel = new JFXPanel();
      frame.add(fxPanel);
      frame.setSize(300, 200);
      frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

      if (!SET_FRAME_VISIBLE_LATE)
      {
         setFrameVisible(frame);
      }

      Platform.runLater(() -> {
                      try {
                      initFX(fxPanel, frame);
                      } catch (Exception e) {}
      });
      if (SET_FRAME_VISIBLE_LATE)
      {
         setFrameVisible(frame);
      }
   }

   private static void initFX(JFXPanel fxPanel, JFrame frame) throws Exception
   {
      // This method is invoked on the JavaFX thread
      Scene scene = createScene();
      fxPanel.setScene(scene);
   }

@andy-goryachev-oracle
Copy link
Contributor

You need to run with this change

Does not help.

I think the problem is that FX stage is being manipulated from the EDT, see the comment in JFXPanel:607 and the FX stage access on JFXPanel:640.

You probably need to wrap the following code in Platform.runLater(), though I can't tell if sendResizeEventToFX(); should be in the EDT or FX app thread:

            if (stage != null) {
               stage.setRenderScaleX(scaleFactorX);
               stage.setRenderScaleY(scaleFactorY);
            }
            sendResizeEventToFX();

@andy-goryachev-oracle
Copy link
Contributor

  1. please merge the master branch
  2. to avoid JDK-8222209 condition, I am launching separate windows on all monitors. What I see is that the label text on the secondary monitor is not as smooth (see earlier screenshots), the image appears as a solid black block. Is this expected?
  3. one interesting thing I noticed is that the master branch launches two windows and it shows O100% on both (even though the primary scale=2, secondary=1). The fixed branch shows O200% on both (again, that appears to be incorrect).

Here is the code:

package goryachev.bugs;

import java.awt.GraphicsConfiguration;
import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;
import java.awt.Rectangle;
import javax.swing.JFrame;
import javax.swing.SwingUtilities;
import javafx.application.Platform;
import javafx.embed.swing.JFXPanel;
import javafx.geometry.Insets;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.layout.Background;
import javafx.scene.layout.BackgroundFill;
import javafx.scene.layout.CornerRadii;
import javafx.scene.layout.HBox;
import javafx.scene.layout.Region;
import javafx.scene.layout.VBox;
import javafx.scene.paint.Color;

public class EmbeddedFrameBug {
    private static final boolean SET_FRAME_VISIBLE_LATE = true;

    private static void initAndShowGUI() //throws Exception
    {
        for (GraphicsDevice d: GraphicsEnvironment.getLocalGraphicsEnvironment().getScreenDevices()) {
            GraphicsConfiguration c = d.getDefaultConfiguration();
            Rectangle r = c.getBounds();

            JFrame frame = new JFrame("Swing and JavaFX");
            final JFXPanel fxPanel = new JFXPanel();
            frame.add(fxPanel);
            frame.setSize(300, 200);
            frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
            frame.setLocation((int)r.getCenterX(), (int)r.getCenterY());

            if (!SET_FRAME_VISIBLE_LATE) {
                setFrameVisible(frame);
            }

            Platform.runLater(() -> {
                try {
                    initFX(fxPanel, frame);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            });
            if (SET_FRAME_VISIBLE_LATE) {
                setFrameVisible(frame);
            }
        }
    }

    private static void initFX(JFXPanel fxPanel, JFrame frame) throws Exception {
        // This method is invoked on the JavaFX thread
        Scene scene = createScene();
        fxPanel.setScene(scene);
    }

    private static void setFrameVisible(JFrame frame) {
        frame.setVisible(true);
    }

    private static Scene createScene() {
        VBox root = new VBox();
        root.setPadding(new Insets(5));
        Scene scene = new Scene(root);

        HBox hBox = new HBox();
        hBox.setPrefSize(30, 30);
        hBox.setMaxWidth(Region.USE_PREF_SIZE);
        hBox.setBackground(new Background(new BackgroundFill(Color.BLACK, CornerRadii.EMPTY, Insets.EMPTY)));

        Label label = new Label();

        scene.windowProperty().addListener((ob, oldWindow, newWindow) -> {
            newWindow.renderScaleXProperty().addListener((obs, oldValue, newValue) -> updateText(label, newValue));
            updateText(label, newWindow.getRenderScaleX());
        });

        root.getChildren().addAll(hBox, label);

        return (scene);
    }

    private static void updateText(Label label, Number scaleX) {
        if (scaleX == null) {
            label.setText("Unknown scale x");
        } else {
            label.setText("O" + String.format("%.0f%%", scaleX.doubleValue() * 100));
        }
    }

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

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Jul 7, 2023

  1. please merge the master branch

    1. to avoid JDK-8222209 condition, I am launching separate windows on all monitors. What I see is that the label text on the secondary monitor is not as smooth (see earlier screenshots), the image appears as a solid black block. Is this expected?

    2. one interesting thing I noticed is that the master branch launches two windows and it shows O100% on both (even though the primary scale=2, secondary=1). The fixed branch shows O200% on both (again, that appears to be incorrect).

  1. It is expected as can be seen in JBS " filled rectangular box shows only black pixels"
  2. Yes, it should show 200% with this current fix. For JDK-8222209, I have a fix for getting different scales on different monitors so that it will show 200% on primary and 100% in secondary but it does not solve the label blurriness on secondary monitor, so could not raise a PR still.

@andy-goryachev-oracle
Copy link
Contributor

are you saying that it should show 200% even for the window that gets shown on the secondary (scale=1)?
even though the test code does listen to the changes on the right scene instance

        scene.windowProperty().addListener((ob, oldWindow, newWindow) -> {
            newWindow.renderScaleXProperty().addListener((obs, oldValue, newValue) -> updateText(label, newValue));
            updateText(label, newWindow.getRenderScaleX());
        });

?

also, the poorly rendered text with the fix - does it mean this PR needs more work?

here is the comparison between the master and the fix, using the window that appears on the secondary:

Screenshot 2023-07-07 at 11 43 49

@prsadhuk
Copy link
Collaborator Author

are you saying that it should show 200% even for the window that gets shown on the secondary (scale=1)? even though the test code does listen to the changes on the right scene instance

        scene.windowProperty().addListener((ob, oldWindow, newWindow) -> {
            newWindow.renderScaleXProperty().addListener((obs, oldValue, newValue) -> updateText(label, newValue));
            updateText(label, newWindow.getRenderScaleX());
        });

?

also, the poorly rendered text with the fix - does it mean this PR needs more work?

here is the comparison between the master and the fix, using the window that appears on the secondary:

Screenshot 2023-07-07 at 11 43 49

Fix to get correct scale in secondary monitor...Poorly rendered text on secondary is tracked in JDK-8222209

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting the scales right on both monitors now (macOS)
the image is rendered with no gaps.
LGTM

@openjdk
Copy link

openjdk bot commented Jul 10, 2023

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274932: Render scales in EmbeddedWindow are not properly updated
8222209: JavaFX is rendered blurry on systems with monitors in different configuration

Co-authored-by: John Hendrikx <jhendrikx@openjdk.org>
Reviewed-by: angorya, kcr, jhendrikx

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the master branch:

  • 7b797b9: 8309558: Create implementation of NSAccessibilityCheckBox protocol
  • e5bb4e1: 8311983: ListView sometimes throws an IndexOutOfBoundsException
  • 72f05a7: 8307536: FileAlreadyExistsException from NativeLibLoader when running concurrent applications with empty cache
  • 5c3e832: 8310885: Width/height of window is not set after calling sizeToScene
  • 3b10562: 8314266: Several test failures after fix for JDK-8159048
  • 9e9c3b3: 8314149: Clipboard does inexact string comparison on mime type
  • c3257fc: 8159048: Animation and AnimationTimer methods must be called on JavaFX Application thread
  • a5183e5: 8283401: ArrayIndexOutOfBoundsException when disconnecting screen(s)
  • bedb964: 8307316: Let JavaFX be built on unknown architectures
  • 8998b2d: 8314141: Missing default for switch in CreateBitmap
  • ... and 23 more: https://git.openjdk.org/jfx/compare/8fef6a3315efed579a431d580aae143da236e7b2...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Jul 10, 2023
@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jul 10, 2023

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@openjdk
Copy link

openjdk bot commented Jul 10, 2023

@prsadhuk this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8274932
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Ready to be integrated labels Jul 10, 2023
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jul 11, 2023
@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Aug 4, 2023

The good:

* image on both monitors (macOS) appear with no gaps

* text on both monitors looks good, anti-aliasing works as expected

The bad:

* both windows (using EmbeddedFrameBug class listed earlier) shows O100% for primary retina screen (should be 200%).  What's more, moving from one screen to another does not trigger the `renderScaleXProperty` change event, using this code:
        scene.windowProperty().addListener((ob, oldWindow, newWindow) -> {
            newWindow.renderScaleXProperty().addListener((obs, oldValue, newValue) -> updateText(label, newValue));
            updateText(label, newWindow.getRenderScaleX());
            newWindow.renderScaleXProperty().addListener((s,p,c) -> {
               System.out.println("w=" + newWindow + " scale=" + c); 
            });
        });

Is this a problem?

I do not have mac setup for FX so cannot comment immediately. it worked for windows where it showed correct scale for both monitors in console and in screen..
(master) ./jdk/bin/java @e://ade/javafx//jfx//rt//build//run.args EmbeddedFrameBug
w=com.sun.javafx.stage.EmbeddedWindow@421cc0cd scale=1.25
w=com.sun.javafx.stage.EmbeddedWindow@18113444 scale=1.75

swing-interop does not have platform-dependant code so not sure why it will behave that way in mac.. I think it's more windows-toolkit issue if renderScaleXProperty listener is not called for scene.windowProperty()

Was that not working before when you first approved? or does it stop working after updateSceneState integration?

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Aug 7, 2023

both windows (using EmbeddedFrameBug class listed earlier) shows O100% for primary retina screen (should be 200%).

@hjohn Seems like @andy-goryachev-oracle is telling it regressed after updateSceneState integration as previously he mentioned

#1171 (review)

getting the scales right on both monitors now (macOS)
the image is rendered with no gaps.
LGTM

So, it seems scaling swing-interop fix withstage.setRenderScaleX/Ywas solving JDK-8274932 but not JDK-8222209 whereas your fix is solving both in windows but not solving JDK-8274932 in mac..
Since this seems to be about windows-toolkit updateSceneState , probably it's best to take it out from this PR and let it have only swing-interop change with stage.setRenderScaleX/Y and remove JDK-8222209 from this PR..
What you both suggest?

@hjohn
Copy link
Collaborator

hjohn commented Aug 7, 2023

both windows (using EmbeddedFrameBug class listed earlier) shows O100% for primary retina screen (should be 200%).

@hjohn Seems like @andy-goryachev-oracle is telling it regressed after updateSceneState integration as previously he mentioned

#1171 (review)

getting the scales right on both monitors now (macOS)
the image is rendered with no gaps.
LGTM

So, it seems scaling swing-interop fix withstage.setRenderScaleX/Ywas solving JDK-8274932 but not JDK-8222209 whereas your fix is solving both in windows but not solving JDK-8274932 in mac.. Since this seems to be about windows-toolkit updateSceneState , probably it's best to take it out from this PR and let it have only swing-interop change with stage.setRenderScaleX/Y and remove JDK-8222209 from this PR.. What you both suggest?

IMHO, calling setRenderScaleX/Y should never be done by JavaFX, unless it is in response to a change in the ouput scale properties -- render scale is for the user to change the render size. Calling them from JFXPanel is fixing a symptom of the problem, not the actual problem. The ouput scales should be correct as well (in fact, in 99% of the cases they will be the same as the render scale, unless the user overrides the render scale).

So, JavaFX should be ensuring that the output scale is correct (which will then be copied to the render scale).

This is why I was looking further, and why I recommended calling updateSceneState, as in that way the output scales get changed, and, consequently, the render scales are updated.

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Aug 7, 2023

both windows (using EmbeddedFrameBug class listed earlier) shows O100% for primary retina screen (should be 200%).

@hjohn Seems like @andy-goryachev-oracle is telling it regressed after updateSceneState integration as previously he mentioned
#1171 (review)

getting the scales right on both monitors now (macOS)
the image is rendered with no gaps.
LGTM

So, it seems scaling swing-interop fix withstage.setRenderScaleX/Ywas solving JDK-8274932 but not JDK-8222209 whereas your fix is solving both in windows but not solving JDK-8274932 in mac.. Since this seems to be about windows-toolkit updateSceneState , probably it's best to take it out from this PR and let it have only swing-interop change with stage.setRenderScaleX/Y and remove JDK-8222209 from this PR.. What you both suggest?

IMHO, calling setRenderScaleX/Y should never be done by JavaFX, unless it is in response to a change in the ouput scale properties -- render scale is for the user to change the render size. Calling them from JFXPanel is fixing a symptom of the problem, not the actual problem. The ouput scales should be correct as well (in fact, in 99% of the cases they will be the same as the render scale, unless the user overrides the render scale).

So, JavaFX should be ensuring that the output scale is correct (which will then be copied to the render scale).

This is why I was looking further, and why I recommended calling updateSceneState, as in that way the output scales get changed, and, consequently, the render scales are updated.

Any idea then why it would not work in macos? Did you test it there? Probably windows-toolkit has a native component where it needs some updation?

@hjohn
Copy link
Collaborator

hjohn commented Aug 7, 2023

both windows (using EmbeddedFrameBug class listed earlier) shows O100% for primary retina screen (should be 200%).

@hjohn Seems like @andy-goryachev-oracle is telling it regressed after updateSceneState integration as previously he mentioned
#1171 (review)

getting the scales right on both monitors now (macOS)
the image is rendered with no gaps.
LGTM

So, it seems scaling swing-interop fix withstage.setRenderScaleX/Ywas solving JDK-8274932 but not JDK-8222209 whereas your fix is solving both in windows but not solving JDK-8274932 in mac.. Since this seems to be about windows-toolkit updateSceneState , probably it's best to take it out from this PR and let it have only swing-interop change with stage.setRenderScaleX/Y and remove JDK-8222209 from this PR.. What you both suggest?

IMHO, calling setRenderScaleX/Y should never be done by JavaFX, unless it is in response to a change in the ouput scale properties -- render scale is for the user to change the render size. Calling them from JFXPanel is fixing a symptom of the problem, not the actual problem. The ouput scales should be correct as well (in fact, in 99% of the cases they will be the same as the render scale, unless the user overrides the render scale).
So, JavaFX should be ensuring that the output scale is correct (which will then be copied to the render scale).
This is why I was looking further, and why I recommended calling updateSceneState, as in that way the output scales get changed, and, consequently, the render scales are updated.

Any idea then why it would not work in macos? Did you test it there? Probably windows-toolkit has a native component where it needs some updation?

Sorry, no, I don't have macos. I'm only pointing out that the original fix was not correct, as you are updating render scale directly but that would leave output scale wrong -- it should be updating the output scale. I've debugged this for the move window to another screen case, but didn't try debugging the initial window showing case.

Are you sure it works correctly on Windows now? In other words, both output and render scale are set correctly? If that still bugs on macos, there may be a 3rd problem that is mac specific.

Judging from the build failure, it does seem to work everywhere except mac, which would point to a deeper underlying problem.

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Aug 7, 2023

Are you sure it works correctly on Windows now?

I have tested EmbeddedFrameBug in windows10 with primary scale 1.25 and secondary scale 1.75 and it worked for me..

@prsadhuk
Copy link
Collaborator Author

prsadhuk commented Aug 7, 2023

@andy-goryachev-oracle @kevinrushforth Since this PR solves issue 2 issues in windows and has not regressed anything in mac (behaves same before and after fix), can we commit this and raise a windows-toolkit JBS issue for mac to see why render scale is not updated for updateSceneState call?

@kevinrushforth
Copy link
Member

Since this PR solves issue 2 issues in windows and has not regressed anything in mac (behaves same before and after fix), can we commit this and raise a windows-toolkit JBS issue for mac to see why render scale is not updated for updateSceneState call?

I'll take a look. HiDPI scaling for retina displays on Mac is somewhat different than on Windows, so it wouldn't surprise me if there was a Mac-specific problem in addition to the platform-independent problems solved by this PR.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have finished my review and testing, and I think this fix is ready to go in. The code changes look good, and the rendering is now visually correct where it wasn't before. We should file a follow-up issue for the problem Andy pointed out (which partially affects Windows as well).

I ran the attached test program on both Mac and Windows with and without this patch on a dual screen configuration with different scales for each screen. Without the patch, the rendering is incorrect on the secondary screen. With the patch the rendering is correct on both platforms.

There are two problems that we need to address with a follow-up issue:

  1. On Mac, the render scale and output scale of the window is reported as 1 on the primary retina screen.
  2. On both Mac and Windows, the render scale and output scale of the window are not updated when the window moves from one screen to another, although it is re-rendered using the correct scale internally. The scales are being updated correctly in JFXPanel, but are not being propagated to the Window.

@openjdk openjdk bot added the ready Ready to be integrated label Aug 8, 2023
}
double newScaleFactorX = config.getDefaultTransform().getScaleX();
double newScaleFactorY = config.getDefaultTransform().getScaleY();
double newScaleFactorX = getCurrentTransform().getScaleX();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor (here and later): unless optimized away by the compiler, this code invokes getCurrentTransform() twice. Would it make sense to introduce a local variable?

AffineTransform t = getCurrentTransform();
double sx = t.getScaleX();
double sy = t.getScaleY();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@openjdk openjdk bot removed the ready Ready to be integrated label Aug 16, 2023
Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks!

@openjdk openjdk bot added the ready Ready to be integrated label Aug 16, 2023
@kevinrushforth
Copy link
Member

@hjohn Do you want a chance to re-review?

@prsadhuk You should be good to integrate this tomorrow (or early next week).

@prsadhuk
Copy link
Collaborator Author

/integrate

@openjdk
Copy link

openjdk bot commented Aug 21, 2023

Going to push as commit 6a7c743.
Since your change was applied there have been 35 commits pushed to the master branch:

  • 2e937bb: 8313956: focusWithin on parents of a newly-added focused node is not updated
  • ddd1f79: 8314212: Crash when loading cnn.com in WebView
  • 7b797b9: 8309558: Create implementation of NSAccessibilityCheckBox protocol
  • e5bb4e1: 8311983: ListView sometimes throws an IndexOutOfBoundsException
  • 72f05a7: 8307536: FileAlreadyExistsException from NativeLibLoader when running concurrent applications with empty cache
  • 5c3e832: 8310885: Width/height of window is not set after calling sizeToScene
  • 3b10562: 8314266: Several test failures after fix for JDK-8159048
  • 9e9c3b3: 8314149: Clipboard does inexact string comparison on mime type
  • c3257fc: 8159048: Animation and AnimationTimer methods must be called on JavaFX Application thread
  • a5183e5: 8283401: ArrayIndexOutOfBoundsException when disconnecting screen(s)
  • ... and 25 more: https://git.openjdk.org/jfx/compare/8fef6a3315efed579a431d580aae143da236e7b2...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 21, 2023
@openjdk openjdk bot closed this Aug 21, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Aug 21, 2023
@openjdk
Copy link

openjdk bot commented Aug 21, 2023

@prsadhuk Pushed as commit 6a7c743.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants