8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows #21980

gredler · 2024-11-08T14:44:18Z

This PR fixes the issue identified in JDK-8148334 in screenshots Page8_landscape.JPG and Page8_portrait.JPG.

It does not address mac_Page1.png or mac_Page8.png, which I'm not even sure are still issues (I have no access to a Mac).

The method in question, PathGraphics.printedSimpleGlyphVector(...) is quite complex, with many special cases being handled in different ways. In this specific scenario (page 8 of PrintTextTest), all special case checks fail, and we fall through all the way to the final handling block, which draws the individual characters one by one. It looks like the problem is that the font transform translation is applied twice, once via the glyph positions, and again by drawString(...) via the font. The proposed fix is to provide drawString(...) a font without any translation transform.

Testing looks good on Linux, but needs to be done on Mac and Windows.

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 2 Reviewers)

Issue

JDK-8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21980

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-08T14:45:11Z

👋 Welcome back gredler! 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 · 2024-11-08T14:46:02Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-11-08T14:46:57Z

@gredler 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.

mlbridge · 2024-11-08T14:50:01Z

Webrevs

gredler · 2024-11-13T17:39:38Z

@honkar-jdk This is the followup to PR #21716 discussed last week. Do you have time to review and possibly test on Windows or mac? The change looks good for me on Linux.

honkar-jdk · 2024-11-13T18:38:48Z

@honkar-jdk This is the followup to PR #21716 discussed last week. Do you have time to review and possibly test on Windows or mac? The change looks good for me on Linux.

Sure. I can check it on windows & macOS and get back to you.

honkar-jdk · 2024-11-15T22:29:31Z

@gredler Tested on windows with print to pdf option. I noticed that the "G..y..h..." line is still being printed above instead of below as seen in the following screenshot. (Left one - without changes, center window - test window, Right one- with the fix).

If you could attach a similar screenshot of your testing on linux, it would be easier to review.

gredler · 2024-11-18T11:13:26Z

@honkar-jdk Interesting, it looks like this fixes Linux then, but not Windows (although the code change is in the shared PathGraphics class). Any idea what Mac looks like on page 8 right now? It looks to me like the Windows screenshot is showing two separate bugs (start position, and then individual glyph advances) -- but that's just a guess.

We may need to create a separate bug in the bug tracker just for Linux page 8 and this PR, since it looks like there are still other issues to be fixed in JDK-8148334.

Linux screenshot:

DamonGuy · 2024-11-18T19:57:08Z

@gredler MacOS looks different. There seems to be some clipping issues or bound issues when printing. In fact, I don't even see the GlyphVector characters being printed. Could not be printed at all or moved somewhere off the page. Really odd. The changes made here don't seem to affect this though since it seems to be an issue both before & after.

gredler · 2024-11-18T22:33:11Z

@DamonGuy Brilliant, thanks for the check. This feels like it might be drawing offscreen because of incorrectly-combined transforms, like what was fixed in PR #20993 (but this would be somewhere in the Mac-specific code).

@honkar-jdk I was able to dig up an old Windows laptop, and have confirmed that the page 8 issue on Windows is caused by two regressions introduced over the past decade but not caught because this is a manual test, not an automated test... I'm going to see if I can't get this test passing on both Linux and Windows. Keep an eye out for additional commits to this PR.

honkar-jdk · 2024-11-19T01:49:50Z

@gredler Tested on windows with the latest fix. The portrait looks good but looks like the fix causes more issues in landscape mode. I'll require some time to go through the source code changes when I get a chance. Looks like you might be in the process of adding few more changes for Windows issue. Let me know once the PR is ready.

Portrait

Landscape

gredler · 2024-11-20T13:15:29Z

@honkar-jdk OK, I think this PR is ready for another review on Windows. The issue you highlighted in your last comment was caused by a partial fix to the JDK-8132890 regression (which had obviously removed the font rotation component, but had less-obviously also removed the device transform rotation component).

The goal is to completely fix this test on Windows and Linux, so I've updated the problem list to flag only macOS as problematic. Will we need a separate bug in the issue tracker, since this PR will not fix the macOS parts of JDK-8148334? I'm not sure whether PRs are allowed to partially fix issues (when they represent more than one bug), or whether they are expected to fully fix whatever issue they reference.

@prrace This is related to two bugs that you worked on in the past: JDK-8132890 and JDK-8029204. It has been a few years, but it would be good to get your review on this as well, if you have the time.

In particular, it feels like we stopped using most of the device transform in some parts of the JDK-8132890 changes (e.g. glyph position calculation), and now we're re-adding the rotation component, and I wonder if there are other parts of the device transform that we are missing, and/or whether it would be easier to go back to using the device transform in the glyph position calculation. (The code seems to work as-is, I'm just wondering if it's more complex than is necessary.)

Summary of changes:

PathGraphics: Do not apply font transform translation component twice (results in incorrect text start point)
WPathGraphics: Do not apply font transform translation component twice (results in incorrect text start point)
WPathGraphics: Include both font and device rotation when calculating glyph positions
WPathGraphics: Fix direction of y scale

honkar-jdk · 2024-11-20T17:48:26Z

@gredler

Will we need a separate bug in the issue tracker, since this PR will not fix the macOS parts of JDK-8148334?

That's correct. I think it is better to create a new JBS bug and use the new JBS ID for this PR than using JDK-8148334. I can create a new JBS issue on your behalf.
Since this test spans across multiple pages testing various glyph transformations it is better to keep JDK-8148334 in open state and reference it while problemlisting the test.

I'm not sure whether PRs are allowed to partially fix issues (when they represent more than one bug), or whether they are expected to fully fix whatever issue they reference.

A PR can have fix that focuses on specific issue and separate PRs can be created to address other issues. So in this case it seems reasonable to have a PR for Windows and Linux fix and create a separate one for macOS.

gredler · 2024-11-20T18:57:26Z

@honkar-jdk I just got access to JBS today, so that's not a problem. I've created JDK-8344637 to track just the Linux and Windows fixes in this PR (scoping out macOS for now). Have you had a chance to review on Windows?

honkar-jdk · 2024-11-20T19:08:14Z

@gredler

Have you had a chance to review on Windows?

Yes, following are the screenshots of Page8 on Windows. Both portrait and landscape look good now although I haven't looked at the other pages in detail (to verify the fix works in all cases and does not produce any side-effects/regressions).

(Right-hand side page => with Fix)

Portrait:

Landscape

honkar-jdk · 2024-11-20T19:16:16Z

@gredler The PR title needs to be updated to the latest JBS title ( I added Page8 to be more specific).

gredler · 2024-11-20T19:30:50Z

@honkar-jdk OK, I've updated the title. I've checked output on both Windows and Linux and I don't think there are any other issues with this test on either platform (on any page).

honkar-jdk

Tested on windows, Pg#8 looks good. I did not notice any regressions on other pages with the fix.

honkar-jdk · 2024-11-26T23:23:59Z

src/java.desktop/share/classes/sun/print/PathGraphics.java

+        Font font2 = font;
+        if (font.isTransformed()) {
+            AffineTransform t = font.getTransform();
+            if ((t.getType() & AffineTransform.TYPE_TRANSLATION) != 0) {
+                t.setTransform(t.getScaleX(), t.getShearY(), t.getShearX(), t.getScaleY(), 0, 0);
+                font2 = font.deriveFont(t);
+            }
+        }
+


The fix to PathGraphics.java looks good.

A new font var is used to copy over all the transforms except translation from the original font object and use new font reference in drawString(). The translation transform is skipped since the drawString takes care of x and y positions.

That's right, this avoids doubling of the text start position coordinates, since the glyph positions which we are about to use already include this translation component. Let me know if you see anything that should be changed, or if I can mark this conversation as resolved.

honkar-jdk · 2024-11-26T23:37:28Z

src/java.desktop/windows/classes/sun/awt/windows/WPathGraphics.java

+             advanceTransform.rotate(devangle); // radians
+             advanceTransform.rotate(iangle * Math.PI / 1800.0); // 1/10 degrees -> radians


Fix to WPathGraphics.java is a little more intricate and certainly requires a more expert review from @prrace.
Was devangle fix added for the landscape issue observed on Pg#8 ?

A earlier comment mentions that device space rotation must not be added.

GDI advances must not include device space rotation. See earlier comment in printGlyphVector() for details.

The two angles (iangle + devangle) need to be used to fix the Windows page 8 printed "GlyphVector with position adjustments" test case, here's what it looks like without these rotation angles (left side is printed and incorrect, right side is not printed and is correct):

Adding iangle alone looks OK in the printed portrait page, but not in the printed landscape page (devangle is zero and therefore makes no difference when printing in portrait mode, but not so for landscape mode).

This restores the rotation transformation that was being applied prior to commit 5c26723 (prior to this commit the advance transform was a copy of the device transform, with the extra iangle rotation applied, meaning that both the device rotation and the iangle rotation were included).

honkar-jdk · 2024-11-26T23:45:56Z

/reviewers 2 reviewer

openjdk · 2024-11-26T23:46:54Z

@honkar-jdk
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 2 Reviewers).

Avoid applying font translation transform twice in some scenarios

489c6dd

openjdk bot added the rfr Pull request is ready for review label Nov 8, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Nov 8, 2024

Comment tweak ('advance' -> 'translation')

f08adea

gredler added 3 commits November 19, 2024 00:05

Fix PrintTextTest page 8 on Windows (regression from JDK-8132890)

c6a7c29

Fix PrintTextTest page 8 on Windows (regression from JDK-8132890)

5edf75e

Fix PrintTextTest page 8 on Windows (regression from JDK-8029204)

105a193

Fix PrintTextTest page 8 on Windows (regression from JDK-8132890)

aeb7b7e

gredler changed the title ~~8148334: The regression case: java/awt/print/PrinterJob/PrintTextTest.java failed as the printed pages do not match the shown pages~~ 8344637: Fix manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows Nov 20, 2024

Add additional issue ID to PrintTextTest bug list

2652e10

gredler changed the title ~~8344637: Fix manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows~~ 8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows Nov 20, 2024

honkar-jdk reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows #21980

8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows #21980

gredler commented Nov 8, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 8, 2024

openjdk bot commented Nov 8, 2024

openjdk bot commented Nov 8, 2024

mlbridge bot commented Nov 8, 2024 •

edited

Loading

gredler commented Nov 13, 2024

honkar-jdk commented Nov 13, 2024

honkar-jdk commented Nov 15, 2024

gredler commented Nov 18, 2024

DamonGuy commented Nov 18, 2024

gredler commented Nov 18, 2024

honkar-jdk commented Nov 19, 2024 •

edited

Loading

gredler commented Nov 20, 2024

honkar-jdk commented Nov 20, 2024 •

edited

Loading

gredler commented Nov 20, 2024

honkar-jdk commented Nov 20, 2024 •

edited

Loading

honkar-jdk commented Nov 20, 2024

gredler commented Nov 20, 2024 •

edited

Loading

honkar-jdk left a comment •

edited

Loading

honkar-jdk Nov 26, 2024 •

edited

Loading

gredler Nov 29, 2024

honkar-jdk Nov 26, 2024 •

edited

Loading

gredler Nov 29, 2024

honkar-jdk commented Nov 26, 2024

openjdk bot commented Nov 26, 2024

		advanceTransform.rotate(devangle); // radians
		advanceTransform.rotate(iangle * Math.PI / 1800.0); // 1/10 degrees -> radians

8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows #21980

Are you sure you want to change the base?

8344637: Fix Page8 of manual test java/awt/print/PrinterJob/PrintTextTest.java on Linux and Windows #21980

Conversation

gredler commented Nov 8, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Nov 8, 2024

openjdk bot commented Nov 8, 2024

openjdk bot commented Nov 8, 2024

mlbridge bot commented Nov 8, 2024 • edited Loading

Webrevs

gredler commented Nov 13, 2024

honkar-jdk commented Nov 13, 2024

honkar-jdk commented Nov 15, 2024

gredler commented Nov 18, 2024

DamonGuy commented Nov 18, 2024

gredler commented Nov 18, 2024

honkar-jdk commented Nov 19, 2024 • edited Loading

gredler commented Nov 20, 2024

honkar-jdk commented Nov 20, 2024 • edited Loading

gredler commented Nov 20, 2024

honkar-jdk commented Nov 20, 2024 • edited Loading

honkar-jdk commented Nov 20, 2024

gredler commented Nov 20, 2024 • edited Loading

honkar-jdk left a comment • edited Loading

Choose a reason for hiding this comment

honkar-jdk Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

gredler Nov 29, 2024

Choose a reason for hiding this comment

honkar-jdk Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

gredler Nov 29, 2024

Choose a reason for hiding this comment

honkar-jdk commented Nov 26, 2024

openjdk bot commented Nov 26, 2024

gredler commented Nov 8, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 8, 2024 •

edited

Loading

honkar-jdk commented Nov 19, 2024 •

edited

Loading

honkar-jdk commented Nov 20, 2024 •

edited

Loading

honkar-jdk commented Nov 20, 2024 •

edited

Loading

gredler commented Nov 20, 2024 •

edited

Loading

honkar-jdk left a comment •

edited

Loading

honkar-jdk Nov 26, 2024 •

edited

Loading

honkar-jdk Nov 26, 2024 •

edited

Loading