Skip to content

JDK-8015739: Background of JInternalFrame is located out of JInternalFrame #10274

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.

Sign up for GitHub

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

Closed
wants to merge 15 commits into from
Closed

JDK-8015739: Background of JInternalFrame is located out of JInternalFrame #10274

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e10aa59
JIF Metal Border Scaling - Initial changes
honkar-jdk Sep 7, 2022
767172e
expanded imports
honkar-jdk Sep 8, 2022
e905d8b
Added test case, minor position changes to shadow lines
honkar-jdk Sep 9, 2022
0968518
updated test case
honkar-jdk Sep 14, 2022
955265c
added test summary
honkar-jdk Sep 14, 2022
b02179c
Merge branch 'master' into JIFBorder_8015739
honkar-jdk Sep 19, 2022
ced69a3
added local variable for scaled corner
honkar-jdk Sep 21, 2022
4047f9d
removed redundant jtreg header
honkar-jdk Sep 30, 2022
6efd841
review changes, saving a scaled version of image
honkar-jdk Oct 4, 2022
d72d6d1
MRI changes
honkar-jdk Oct 4, 2022
827a348
review changes
honkar-jdk Oct 4, 2022
2e3b7af
test updated
honkar-jdk Oct 5, 2022
da54767
fix for pixel color at edge, outermost border lines drawn as part of …
honkar-jdk Oct 7, 2022
99664b3
highlight and shadow line positioning changes
honkar-jdk Oct 20, 2022
4fa3490
new x,y and translate component added to fix pixel painting issue
honkar-jdk Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 132 additions & 43 deletions src/java.desktop/share/classes/javax/swing/plaf/metal/MetalBorders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -25,22 +25,44 @@

package javax.swing.plaf.metal;

import javax.swing.*;
import javax.swing.border.*;
import javax.swing.plaf.*;
import sun.swing.StringUIClientPropertyKey;
import sun.swing.SwingUtilities2;

import javax.swing.AbstractButton;
import javax.swing.ButtonModel;
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 3, 2022

Choose a reason for hiding this comment

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

I'm unsure if we all agree on the order of imports in client area, however, the common order is

import java.*;
// optional blank line
import javax.*;
// blank line
import sun.*; // other packages
// blank line
import static *; // if applicable

import javax.swing.JButton;
import javax.swing.JComponent;
import javax.swing.JInternalFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JToolBar;
import javax.swing.SwingConstants;
import javax.swing.SwingUtilities;
import javax.swing.UIManager;
import javax.swing.border.AbstractBorder;
import javax.swing.border.Border;
import javax.swing.border.CompoundBorder;
import javax.swing.border.EmptyBorder;
import javax.swing.border.LineBorder;
import javax.swing.border.MatteBorder;
import javax.swing.plaf.BorderUIResource;
import javax.swing.plaf.UIResource;
import javax.swing.plaf.basic.BasicBorders;
import javax.swing.text.JTextComponent;

import java.awt.Component;
import java.awt.Insets;
import java.awt.BasicStroke;
import java.awt.Color;
import java.awt.Component;
import java.awt.Dialog;
import java.awt.Frame;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.Insets;
import java.awt.Stroke;
import java.awt.Window;

import sun.swing.StringUIClientPropertyKey;
import sun.swing.SwingUtilities2;
import java.awt.geom.AffineTransform;


/**
Expand Down Expand Up @@ -223,10 +245,21 @@ public static class InternalFrameBorder extends AbstractBorder implements UIReso
* Constructs a {@code InternalFrameBorder}.
*/
public InternalFrameBorder() {}
/**
* Round the double to nearest integer, make sure we round
* to lower integer value for 0.5
*
* @param d number to be rounded
* @return a {@code int} which is the rounded value of provided number
*/
private static int roundDown(double d)
{
double decP = (Math.ceil(d) - d);
return (int)((decP == 0.5) ? Math.floor(d) : Math.round(d));
}

public void paintBorder(Component c, Graphics g, int x, int y,
int w, int h) {

int w, int h) {
Color background;
Color highlight;
Color shadow;
Expand All @@ -241,41 +274,97 @@ public void paintBorder(Component c, Graphics g, int x, int y,
shadow = MetalLookAndFeel.getControlInfo();
}

g.setColor(background);
// Draw outermost lines
g.drawLine( 1, 0, w-2, 0);
g.drawLine( 0, 1, 0, h-2);
g.drawLine( w-1, 1, w-1, h-2);
g.drawLine( 1, h-1, w-2, h-1);
Graphics2D g2d = (Graphics2D) g;
Copy link
Member

@azuev-java azuev-java Sep 20, 2022

Choose a reason for hiding this comment

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

Regardless, i see that absolutely similar code down this file for FrameBorder and DialogBorder. Are these UI elements suffer from the same issue or is it only limited to InternalFrame? If they do suffer from the same issue i would suggest either fixing it all together or at least submitting new bugs tracking existence of the problem. Of course problem might not be present for some reason in these components but it is worth checking it out while you are working on the code and remember all the details.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Sep 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@azuev-java Sure. I'll check if there is any observable distortion in other components and either make changes as part of this PR or track it under JDK-8282958. Currently there exists a generic umbrella issue created for border scaling issue, all the related issues are linked to it. The plan is to create a utility method (for common steps) that can be reused across components where this border rendering issue is observed.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@azuev-java Tested FrameBorder and DialogBorder, they have a similar problem to JInternalFrameBorder (border scaling issue). I have created a new JBS issue - JDK-8294484 to track Frame and Dialog Borders of Metal LAF as there is a plan to create a common utility method and refactor the common steps.

Copy link
Member

@aivanov-jdk aivanov-jdk Sep 28, 2022

Choose a reason for hiding this comment

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

Thank you, @honkar-jdk. Once the border handling is unified, fixing FrameBorder and DialogBorder will be easier and code duplication will be avoided.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Sep 30, 2022

Choose a reason for hiding this comment

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

Refactoring to create a common utility method is being tracked by - JDK-8294680

Copy link
Member

@mrserb mrserb Oct 7, 2022

Choose a reason for hiding this comment

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

It could be a DebugGraphics which is not a Graphics2D

Copy link
Member

@mrserb mrserb Nov 5, 2022

Choose a reason for hiding this comment

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

If the code will be refactored please double-check that the blind cast will not be used, "g" could DebugGraphics which is not a Graphics2D

Copy link
Contributor Author

@honkar-jdk honkar-jdk Nov 7, 2022

Choose a reason for hiding this comment

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

Thank you for catching it. Made a note of it , to add it during refactoring changes - JDK-8294680

AffineTransform at = g2d.getTransform();
Stroke oldStk = g2d.getStroke();
Color oldColor = g2d.getColor();
boolean resetTransform;
int stkWidth = 1;

// if m01 or m10 is non-zero, then there is a rotation or shear
// skip resetting the transform
resetTransform = (at.getShearX() == 0) && (at.getShearY() == 0);

if (resetTransform) {
Copy link
Member

@mrserb mrserb Oct 7, 2022

Choose a reason for hiding this comment

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

Please clarify why we should reset the current transform? How it will work if the user sets a transform and render this to the BufferedImage.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Oct 7, 2022

Choose a reason for hiding this comment

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

On fractional scaling, painting of lines and borders have issues (lines seem to be separated or look uneven) as shown above, #10274 (comment). In order to paint them uniformly and next to each other, the idea is to render at 1x scaling, apply the required thickness/scaling to the lines at this point (when transform is reset) and then the original (old) transform is restored at the end.

This idea was referenced from a similar issue #7449 (comment).

g2d.setTransform(new AffineTransform());
stkWidth = roundDown(Math.min(at.getScaleX(), at.getScaleY()));
g2d.setStroke(new BasicStroke((float) stkWidth));
}

// Draw the bulk of the border
for (int i = 1; i < 5; i++) {
g.drawRect(x+i,y+i,w-(i*2)-1, h-(i*2)-1);
}
int xtranslation = 0;
int ytranslation = 0;
int width = 0;
int height = 0;

if (c instanceof JInternalFrame &&
((JInternalFrame)c).isResizable()) {
g.setColor(highlight);
// Draw the Long highlight lines
g.drawLine( corner+1, 3, w-corner, 3);
g.drawLine( 3, corner+1, 3, h-corner);
g.drawLine( w-2, corner+1, w-2, h-corner);
g.drawLine( corner+1, h-2, w-corner, h-2);

g.setColor(shadow);
// Draw the Long shadow lines
g.drawLine( corner, 2, w-corner-1, 2);
g.drawLine( 2, corner, 2, h-corner-1);
g.drawLine( w-3, corner, w-3, h-corner-1);
g.drawLine( corner, h-3, w-corner-1, h-3);
}
if (resetTransform) {
width = roundDown(at.getScaleX() * w);
height = roundDown(at.getScaleY() * h);
xtranslation = roundDown(at.getScaleX() * x + at.getTranslateX());
ytranslation = roundDown(at.getScaleY() * y + at.getTranslateY());
} else {
width = w;
height = h;
xtranslation = x;
ytranslation = y;
}
g2d.translate(xtranslation, ytranslation);

}
// border and corner scaling
int scaledCorner = (int) Math.round(corner * at.getScaleX());
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 3, 2022

Choose a reason for hiding this comment

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

Just a suggestion, you can call this variable corner but make the constant above upper case CORNER. This way the old code would remain basically the same and continue using corner (which was previously a field).

Copy link
Contributor Author

@honkar-jdk honkar-jdk Oct 4, 2022

Choose a reason for hiding this comment

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

Updated

// loop constraint for bulk of the border
int loopCount = (int) Math.round(5 * at.getScaleX());

public Insets getBorderInsets(Component c, Insets newInsets) {
newInsets.set(5, 5, 5, 5);
return newInsets;
}
// midpoint at which highlight & shadow lines
// are positioned on the border
int midPoint = loopCount/2;

g.setColor(background);
// Draw outermost lines
g.drawLine( 0, 0, width-1, 0);
g.drawLine( 0, 1, 0, height-1);
g.drawLine( width-1, 1, width-1, height-1);
g.drawLine( 1, height-1, width-1, height-1);
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 3, 2022

Choose a reason for hiding this comment

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

I suggest removing the space after the opening parenthesis.

There's usually a space on either side of binary operators, the - in this case.

I believe this is existing code which was just moved around, so maybe its style is not worth touching.


// Draw the bulk of the border
for (int i = 1; i <= loopCount; i++) {
Copy link
Contributor

@alisenchung alisenchung Oct 3, 2022

Choose a reason for hiding this comment

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

Should we redo the drawing of this border to use Graphics2D fillRect instead?

Copy link
Contributor Author

@honkar-jdk honkar-jdk Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That was my initial thought too. But, as the loopCount won't be really large (Eg. it is 15 for 300% scaling) and it wouldn't affect the performance drastically, I have retained the original approach to draw the bulk of the border.

Copy link
Member

@aivanov-jdk aivanov-jdk Oct 4, 2022

Choose a reason for hiding this comment

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

Let us leave it as is at this time. There are enough changes.

At the same time, it's a good suggestion for optimization. We should consider it in a separate CR.

Copy link
Contributor Author

@honkar-jdk honkar-jdk Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this code can be improved further using fillRect() (as a separate PR).

g.drawRect(x+i,y+i,width-(i*2)-1, height-(i*2)-1);
}

if (c instanceof JInternalFrame && ((JInternalFrame)c).isResizable()) {
// Draw the Long highlight lines
g.setColor(highlight);
g.drawLine(scaledCorner + 1, midPoint+stkWidth,
Copy link
Contributor

@alisenchung alisenchung Oct 3, 2022

Choose a reason for hiding this comment

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

The spacing here and when drawing shadow lines is a bit inconsistent

Copy link
Contributor Author

@honkar-jdk honkar-jdk Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@alisenchung I tried by increasing and decreasing the line position a bit, the current combination provided optimal positioning of both the shadow and highlight line within the border for all the scales. The slight inconsistency in spacing is probably due to rounding losses in loop count, stroke width and corner. I'll check it again.

Copy link
Contributor

@alisenchung alisenchung Oct 4, 2022

Choose a reason for hiding this comment

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

Oh, I meant the actual code spacing, theres a space between scaledCorner and 1, but not space between midPoint and stkWidth

width- scaledCorner, midPoint+stkWidth); //top
g.drawLine(midPoint+stkWidth, scaledCorner + 1,
midPoint+stkWidth, height- scaledCorner); //left
g.drawLine(width-midPoint, scaledCorner + 1,
width-midPoint, height- scaledCorner); //right
g.drawLine(scaledCorner + 1, height-midPoint,
width- scaledCorner, height-midPoint); //bottom

// Draw the Long shadow lines
g.setColor(shadow);
g.drawLine(scaledCorner, midPoint, width- scaledCorner -1, midPoint);
g.drawLine(midPoint, scaledCorner, midPoint, height- scaledCorner -1);
g.drawLine(width-(midPoint+stkWidth), scaledCorner,
width-(midPoint+stkWidth), height- scaledCorner -1);
g.drawLine(scaledCorner, height-(midPoint+stkWidth),
width- scaledCorner -1, height-(midPoint+stkWidth));
}

// Undo the resetTransform setting from before
g2d.translate(-xtranslation, -ytranslation);
if (resetTransform) {
g2d.setColor(oldColor);
g2d.setTransform(at);
g2d.setStroke(oldStk);
}
}

public Insets getBorderInsets(Component c, Insets newInsets) {
newInsets.set(5, 5, 5, 5);
return newInsets;
}
}

/**
Expand Down
Loading