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

4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize #433

Closed
wants to merge 1 commit into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Sep 30, 2020

Please review a fix for an issue where minimumLayoutSize and preferredlayoutSize of MetalRootLayout class wrongly uses the width of the title pane in the height calculation:
Proposed fix is to rectify the anomaly and use tpHeight for height calculation.

All closed, open jtreg and JCK tests and SwingSet2 Metal L&F are unaffected by this change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize

Download

$ git fetch https://git.openjdk.java.net/jdk pull/433/head:pull/433
$ git checkout pull/433

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 30, 2020

👋 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 Pull request is ready for review label Sep 30, 2020
@openjdk
Copy link

openjdk bot commented Sep 30, 2020

@prsadhuk The following label will be automatically applied to this pull request:

  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the swing client-libs-dev@openjdk.org label Sep 30, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 30, 2020

Webrevs

@@ -536,7 +536,7 @@ public Dimension minimumLayoutSize(Container parent) {
}

return new Dimension(Math.max(Math.max(cpWidth, mbWidth), tpWidth) + i.left + i.right,
cpHeight + mbHeight + tpWidth + i.top + i.bottom);
cpHeight + mbHeight + tpHeight + i.top + i.bottom);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to check this change by some testcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried at the time of PR but unable to check as MetalRootLayout is private so not able to access its methods. That is why I ran all client-tier3 jtreg and jck tests, and mach5 job link is posted in JBS.

Copy link
Member

Choose a reason for hiding this comment

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

If the MetalRootLayout is private and it is not possible to trigger this code in other ways, then it means we can delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MetalRootLayout class and methods are private but it is called from MetalRootPaneUI so we cannot delete it
but I could not find any ways to invoke MetalRootLayout methods from testcase.

Copy link
Member

Choose a reason for hiding this comment

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

The MetalRootPaneUI installs the MetalRootLayout as a layout manager for the current JRootPane. And it should be used from the Container.preferredSize()

Copy link
Member

Choose a reason for hiding this comment

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

You need to set decorations for the root pane:

        UIManager.setLookAndFeel(new MetalLookAndFeel());
        JFrame frame = new JFrame();
        frame.getRootPane().setWindowDecorationStyle(JRootPane.FRAME);
        frame.pack();
        System.out.println("Layout: " + frame.getRootPane().getLayout());
========
Layout: javax.swing.plaf.metal.MetalRootPaneUI$MetalRootLayout@69d9c55

Copy link
Contributor Author

@prsadhuk prsadhuk Nov 5, 2020

Choose a reason for hiding this comment

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

I tried with

 UIManager.setLookAndFeel(new MetalLookAndFeel());
JFrame frame = new JFrame();
                frame.setSize(new Dimension(500, 800));
                frame.getRootPane().setWindowDecorationStyle(JRootPane.FRAME);
                //frame.pack();
                frame.setVisible(true);
                System.out.println("Layout: " + frame.getRootPane().getLayout());
                System.out.println("PreferredSize " + frame.getRootPane().getPreferredSize());

but it seems tpWidth and tpHeight what is being changed is the width and height of MetalTitlePane which seems always comes as 23 for both no matter what frame size is set, so I guess using any variable will do but logically, it will be good to use "tpHeight" for height calculation.
If it is believed to continue with same logical but not technical error, I will close this PR without integrating.

Copy link
Member

Choose a reason for hiding this comment

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

The bug submitter said that it is somehow affected his application. So I assume there is a way to trigger this bug in the MetalRootLayout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From MetalTitlePane.java, the same "height" is being used for width and height so the value is same for both.

private class TitlePaneLayout implements LayoutManager {
        public void addLayoutComponent(String name, Component c) {}
        public void removeLayoutComponent(Component c) {}
        public Dimension preferredLayoutSize(Container c)  {
            int height = computeHeight();
            return new Dimension(height, height);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no reproducible testcase and the submitter id is not active anymore so cannot ask him and I am not sure of any other way, so if this is not accepted without a reproducing testcase, I will close this PR.

@prsadhuk prsadhuk closed this Nov 6, 2020
@prsadhuk prsadhuk deleted the JDK-4916923 branch November 25, 2020 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Pull request is ready for review swing client-libs-dev@openjdk.org
2 participants