Skip to content

Conversation

@alisenchung
Copy link
Contributor

@alisenchung alisenchung commented Feb 11, 2022

Changed the drawing area to be increased by 0.5 on the left side to prevent clipping


Progress

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

Issue

  • JDK-8279614: The left line of the TitledBorder is not painted on 150 scale factor

Reviewers

Contributors

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7449

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/7449.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 11, 2022

👋 Welcome back achung! 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
Copy link

openjdk bot commented Feb 11, 2022

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

  • client

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.

System.out.println("Color " + buff.getRGB(21, 80));
saveImage(buff, "test.png");
throw new RuntimeException("Border was clipped or overdrawn.");
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we draw directly to the buffered image using the scaled graphics to reproduce the bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, the test fails before the fix, and when inspecting the saved image it looks identical to the actual frame.

@@ -0,0 +1,97 @@
import java.awt.BorderLayout;
Copy link
Member

Choose a reason for hiding this comment

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

To prevent all this jcheck issue you can install the jcheck hook locally.

path.append(new Rectangle(borderX, labelY, labelX - borderX - TEXT_SPACING, labelH), false);
path.append(new Rectangle(labelX + labelW + TEXT_SPACING, labelY, borderX - labelX + borderW - labelW - TEXT_SPACING, labelH), false);
path.append(new Rectangle(borderX, labelY + labelH, borderW, borderY - labelY + borderH - labelH), false);
path.append(new Rectangle2D.Float((float) borderX, borderY, borderW, labelY - borderY), false);
Copy link
Member

Choose a reason for hiding this comment

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

Why the current coordinates do not work, is it because we calculate them in the wrong way or probably the insets are wrong?

Copy link
Contributor Author

@alisenchung alisenchung Feb 14, 2022

Choose a reason for hiding this comment

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

I think there's a rounding error when doing x.5 scalings causing the drawing area to be one pixel too small on the left side

Copy link
Member

Choose a reason for hiding this comment

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

Can you please check how it will work for the undecorated frame, if it will be fine then the problem is somehow related to the size of the insets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The undecorated frame has no problems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrserb I've been trying to manually adjust the insets/variables to see what could be causing the issue, but only decreasing borderX by 1 seems to fix the issue. Could you clarify what you can gather from the undecorated frame working?

@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 14, 2022
@mlbridge
Copy link

mlbridge bot commented Feb 14, 2022

@alisenchung
Copy link
Contributor Author

I've updated the fix to stop overdrawing, please review @mrserb @prrace @prsadhuk

@prrace
Copy link
Contributor

prrace commented Mar 4, 2022

Can you explain (walk through) why the old code was wrong and the new code is correct ?
There's a lot of things that aren't obvious to me.
The old code would use the same coords and just toggle which parts were in highlight/shadow
Now you've spilit is so that lowered does things in a different order and so forth and I find it
impossible without an explanation of your reasoning to say if that reasoning is correct ...

Also the bug report deserves an evaluation.

@alisenchung
Copy link
Contributor Author

Can you explain (walk through) why the old code was wrong and the new code is correct ? There's a lot of things that aren't obvious to me. The old code would use the same coords and just toggle which parts were in highlight/shadow Now you've spilit is so that lowered does things in a different order and so forth and I find it impossible without an explanation of your reasoning to say if that reasoning is correct ...

Also the bug report deserves an evaluation.

I think currently the problem is the lighter color is overdrawing the darker color, so I swapped the drawing order so that the darker color is always drawn second. I think the reason the rect and lines are drawn at the same x value probably has to do with the affine transform in the graphics object, but I'm not sure why it happens.

@alisenchung
Copy link
Contributor Author

I just added the functions Phil suggested, @azuev-java could you also review?

Copy link
Member

@azuev-java azuev-java left a comment

Choose a reason for hiding this comment

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

Latest version looks Ok to me.

@openjdk
Copy link

openjdk bot commented Mar 14, 2022

@alisenchung 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:

8279614: The left line of the TitledBorder is not painted on 150 scale factor

Co-authored-by: Alexey Ivanov <[email protected]>
Reviewed-by: kizune, aivanov, prr

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 2031 new commits pushed to the master branch:

  • 53a0ace: 8286101: Support formatting in @value tag
  • 8f400b9: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true'
  • e0baf01: 8287007: [cgroups] Consistently use stringStream throughout parsing code
  • 1769596: 8285263: Minor cleanup could be done in java.security
  • b97a4f6: 8288114: Update JIRA link in vcs.xml
  • 2adef6a: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
  • 4aede33: 8288282: Zero-release build is broken after JDK-8279047 due to UseHeavyMonitors is read-only
  • 0207d76: 8287926: AArch64: intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long
  • 33ed036: 8283775: better dump: VM support for graph querying in debugger with BFS traversal and node filtering
  • ac28be7: 8283612: IGV: Remove Graal module
  • ... and 2021 more: https://git.openjdk.org/jdk/compare/ab490534a1d14ad48ceb532ec1673ca9636f552d...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@azuev-java, @aivanov-jdk, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 14, 2022
Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

The test uses 2 spaces for indentation. It should use 4 spaces for each level of indentation.

import java.awt.Insets;
import java.awt.Rectangle;
import java.awt.geom.Path2D;
import java.awt.geom.Rectangle2D;
Copy link
Member

Choose a reason for hiding this comment

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

This import isn't used. There are no other changes to this file but this added import.

In addition to it, you can also remove java.beans.PropertyChangeEvent from imports which is unused as well.

Copy link
Member

Choose a reason for hiding this comment

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

Either revert the changes to TitledBorder or remove the two unused imports: java.awt.geom.Rectangle2D and java.beans.PropertyChangeEvent.

public static JPanel childPanel;

public static void main(String[] args) throws Exception {
LookAndFeelInfo laf = UIManager.getInstalledLookAndFeels()[3];
Copy link
Member

Choose a reason for hiding this comment

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

Is the test needs to be run in a specific LookAndFeel? If so, you have to set explicitly rather than rely on the order of LAFs.

Comment on lines 63 to 68
BufferedImage buff = new BufferedImage(frame.getWidth()*2, frame.getHeight()*2,
BufferedImage.TYPE_INT_ARGB);
Graphics2D graph = buff.createGraphics();
graph.scale(1.5, 1.5);
frame.paint(graph);
graph.dispose();
Copy link
Member

Choose a reason for hiding this comment

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

If we paint to BufferedImage, it makes sense to bypass creating and showing the frame, a panel with TitledBorder will be enough. In this case, the test can be headless.

Shall we run the test with a set of scales: 1.00, 1.25, 1.50, 1.75, 2.00?

Without the fix, the left line looks clipped at 2.0 scale too: the shadow is 1-pixel wide on the left whereas the highlight is 2-pixel wide on the four edges.

boolean testFail = true;
for (int i = 15; i < 25 && testFail == true; i++) {
for (int j = 80; j < 100; j++) {
if (buff.getRGB(i, j) == -6250336) {
Copy link
Member

Choose a reason for hiding this comment

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

Using hex for colors is better.

Getting the highlight or shadow color from the border would make the test more generic. If the color used by border changes, the test will fail but it shouldn't.

try {
ImageIO.write(image, "png", new File(filename));
} catch (IOException e) {
// Don’t propagate the exception
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 14, 2022

Choose a reason for hiding this comment

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

I recommend using a regular apostrophe (') instead of typographic one (’). It shouldn't cause issues with compiling because it's in the comment, yet the character may be displayed incorrectly if the default system encoding doesn't match the encoding of the file.

Comment on lines 153 to 154
// 8279614: The dark line was being overdrawn by the light line. The fix was to
// make sure that the dark line is always drawn second.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 8279614: The dark line was being overdrawn by the light line. The fix was to
// make sure that the dark line is always drawn second.
// 8279614: The dark line was overdrawn by the light line.
// The fix makes sure that the dark line is always drawn second.

Does the opposite never happen? That the shadow overdraws the highlight.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a comment on the actual comment rather than whether the intent is correct.
I'm not in favour of putting bug ids into the comments. Our code will end up utterly cluttered with them
Also it is phrased to describe a fix.
So IF it were to stand as is, I would have phrased it as "what we are doing now and why", ie
// Make sure the shadow line always is drawn second because it needs to over draw the border.

Copy link
Member

Choose a reason for hiding this comment

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

I should've posted my question separately rather than as a comment to the comment.

@aivanov-jdk
Copy link
Member

Can you explain (walk through) why the old code was wrong and the new code is correct ? There's a lot of things that aren't obvious to me. The old code would use the same coords and just toggle which parts were in highlight/shadow Now you've spilit is so that lowered does things in a different order and so forth and I find it impossible without an explanation of your reasoning to say if that reasoning is correct ...
Also the bug report deserves an evaluation.

I think currently the problem is the lighter color is overdrawing the darker color, so I swapped the drawing order so that the darker color is always drawn second. I think the reason the rect and lines are drawn at the same x value probably has to do with the affine transform in the graphics object, but I'm not sure why it happens.

Is it possible that in the new order the darker colour is drawn over the lighter one?

@alisenchung
Copy link
Contributor Author

I think you're right that now the darker colour is drawing over the lighter one. I'm currently changing the drawing of the border to take into account the scaling to prevent either color from overdrawing.

@aivanov-jdk
Copy link
Member

aivanov-jdk commented Mar 14, 2022

I believe the issue is broader. I modified the test to display four panels, each panel is moved pixel to the left. The screenshots below are taken with the current JDK code without Alisen's fix.

125%
Border painting @ 125%

150%
Border painting @ 150%

When uiScale=1.25, you can see there's background color between the shadow and highlight on the second panel. The same problem is seen for the upper border. When uiScale=1.5, you can see that some lines are 2-pixel wide and some are 1-pixel wide.

I believe it's because of rounding. I don't know how we can make sure the lines are always drawn next to each other and always have the same thickness.

@lukeu
Copy link

lukeu commented Mar 15, 2022

Good catch. I'm not in a position to build this patch right now, but I suspect that if you put such a border in the right-hand-side of a splitter pane, then as you drag the splitter's divider, the light lines would still be jumping in and out from underneath the dark ones and/or 'jiggling' in width. (When tested at 125%, 175% etc)

I believe it's because of rounding. I don't know how we can make sure the lines are always drawn next to each other and always have the same thickness.

I believe the only way to get pixel-perfect (& balanced weight) rendering of 2 lines next to each other at non-integer scale factors, when using integer based APIs like drawLine, will be to undo the affine transformation upon the pre-scaled Graphics2D object, and render it at 1x scaling. Then you'd need to decide how to compute the desired line thickness (like: should it jump 1px > 2px at 150% scaling or 175% scaling?)

(IMHO, the JDK-9+ approach of passing in a pre-scaled Graphics object is a bit of a coarse approximation - useful for components that haven't natively considered HiDPI yet. I'm happy to see the built-in L&F code being improved in cases like this; while noting that there are many more.)

@lukeu
Copy link

lukeu commented Mar 16, 2022

OK after many hours of "bash"ing, I've convinced the JDK, cygwin and VS-2017 to play nicely again :-)

My suspicions were close: the lines jump apart, rather than overlap at 125%, and jiggle between 1 & 2px at 175%. The following screenshot is with the patched code at 125% display scaling on Windows, then zoomed in 3x:

JDK-8279614-patched-zoom_x3

Each subsequent titled border is offset 1 extra pixel (as given in the code) to the right, and you can see only the 3rd lets the background between the lines on the left side, at this particular splitter position. If it might be of use, here's my interactive test harness:

import java.awt.*;
import javax.swing.*;

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

    private void run() {
        try {
            UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
        } catch (Exception e) {
            e.printStackTrace();
        }
        var frame = new JFrame();
        frame.getContentPane().add(createSplitters());
        frame.setSize(new Dimension(800, 600));
        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        frame.setVisible(true);
    }

    private JSplitPane createSplitters() {
        return createSplit(
                JSplitPane.HORIZONTAL_SPLIT,
                new JPanel(),
                createSplit(
                        JSplitPane.VERTICAL_SPLIT,
                        new JPanel(),
                        createTestArea()));
    }

    private JSplitPane createSplit(int dir, JComponent left, JComponent right) {
        var split = new JSplitPane(dir, true, left, right);
        split.setDividerLocation(200);
        return split;
    }

    private Box createTestArea() {
        Box box = Box.createVerticalBox();
        box.add(createBorderedCheckbox(1));
        box.add(createBorderedCheckbox(2));
        box.add(createBorderedCheckbox(3));
        return box;
    }

    private JComponent createBorderedCheckbox(int i) {
        JPanel childPanel = new JPanel(new BorderLayout());
        childPanel.setBorder(BorderFactory.createCompoundBorder(
                BorderFactory.createEmptyBorder(0, i, 0, 0),
                BorderFactory.createTitledBorder("Title " + i)));
        childPanel.add(new JCheckBox(), BorderLayout.CENTER);
        childPanel.setBackground(new Color(190, 190 + 20 * i , 190));
        return childPanel;
    }
}

Comment on lines 172 to 173
RenderingHints rend =
new RenderingHints(RenderingHints.KEY_STROKE_CONTROL, RenderingHints.VALUE_STROKE_PURE);
Copy link
Member

Choose a reason for hiding this comment

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

If using VALUE_STROKE_PURE changes nothing, should it be removed then?

Moreover, the hints aren't applied to anything.


g.translate(-x, -y);
// Set the transform we removed earlier
if (g instanceof Graphics2D) {
Copy link
Member

Choose a reason for hiding this comment

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

The condition should be

Suggested change
if (g instanceof Graphics2D) {
if (resetTransform) {

If resetTransform is true, then g is an instance of Graphics2D.

If resetTransform is false, there's nothing to restore anyway.

* @test
* @bug 8279614
* @summary The left line of the TitledBorder is not painted on 150 scale factor
* @requires (os.family == "windows")
Copy link
Member

Choose a reason for hiding this comment

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

It's still unclear to me whether we leave the requirement for Windows only or remove it. It came up a few times but no clear decision has been taken.

The change is not Windows-specific, the test is not Windows-specific, it can be run on other platforms. However, fractional UI scales aren't supported on other platforms but Windows; at the same time, applying fractional scales to Graphics when painting to a BufferedImage is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it sufficient to make it Windows-specific at this time.
The BufferedImage case is extremely uncommon.
Mac is integer only and we don't have any timeline for supporting fractional scaling on Linux

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable. Let's leave it Windows-specific.

Copy link
Member

@aivanov-jdk aivanov-jdk 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 to me.

The question about limiting the test to Windows only is still open.

Comment on lines 156 to 158
AffineTransform at = new AffineTransform();
Stroke oldStk = new BasicStroke();
int stkWidth = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Now the usage of these variables is restricted to the case where resetTransform is set to true. Therefore, they can be left uninitialised here; a value is assigned before it's used in all the code paths as far as I can see.

It's just a small (premature) optimisation, it avoids creating two objects. I don't insist though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be inclined to remove the initialisation.

Copy link
Member

@aivanov-jdk aivanov-jdk Jun 10, 2022

Choose a reason for hiding this comment

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

I see, the static analyser is not smart enough to notice that the variables get initialised before they're used.

To resolve the error, initialise at and oldStk with null.

stkWidth needs to be initialised by 1 as it's currently done, its value is used even if resetTransform remains false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, seems the code logic is too much for the compiler
intuitively I'd have expected that it should be, but you'd need to restructure to fix it ..

import java.awt.Rectangle;
import java.awt.Color;
import java.awt.Component;
import java.awt.RenderingHints;
Copy link
Member

Choose a reason for hiding this comment

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

RenderingHints aren't used, to be removed.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

Please remove import java.awt.RenderingHints; from EtchedBorder.java.

@alisenchung
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 13, 2022
@openjdk
Copy link

openjdk bot commented Jun 13, 2022

@alisenchung
Your change (at version d4ad493) is now ready to be sponsored by a Committer.

@aivanov-jdk
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Jun 13, 2022

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

  • 9b6d0a7: 8285838: DST not applying properly with zone id offset set with TZ env variable
  • 53a0ace: 8286101: Support formatting in @value tag
  • 8f400b9: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true'
  • e0baf01: 8287007: [cgroups] Consistently use stringStream throughout parsing code
  • 1769596: 8285263: Minor cleanup could be done in java.security
  • b97a4f6: 8288114: Update JIRA link in vcs.xml
  • 2adef6a: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
  • 4aede33: 8288282: Zero-release build is broken after JDK-8279047 due to UseHeavyMonitors is read-only
  • 0207d76: 8287926: AArch64: intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long
  • 33ed036: 8283775: better dump: VM support for graph querying in debugger with BFS traversal and node filtering
  • ... and 2022 more: https://git.openjdk.org/jdk/compare/ab490534a1d14ad48ceb532ec1673ca9636f552d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 13, 2022
@openjdk openjdk bot closed this Jun 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jun 13, 2022
@openjdk
Copy link

openjdk bot commented Jun 13, 2022

@aivanov-jdk @alisenchung Pushed as commit b42c1ad.

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

rajamah added a commit to rajamah/jdk that referenced this pull request Oct 11, 2022
…systems

This fix is based on a similar approach as described here openjdk#7449 (comment).
We rescale the line border to render the textfield border correctly, so that all sides of the textfield border are of same thickness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client [email protected] integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

7 participants