-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8307246 : Printing: banded raster path doesn't account for device offset values #17030
Conversation
…SICALOFFSETY of device for banded-raster printing loop
👋 Welcome back vtstydev! A progress list of the required criteria for merging this PR into |
Webrevs
|
This is going to require manual verification. Likely we'll need to test all possible page orientations too - did you try that ? |
The PR title needs to change to match the bug. |
I printed some documents, that renders with banded-raster printing loop, both orientations on Windows and Linux. They were printed well. |
@vtstydev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
@vtstydev This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@prrace The system is about to close this PR. Is there a chance for your time to be taken for it? |
I was hoping (!) you would write a test. In fact there needs to be one. I've not done those last 3 things but here's a test which starts with what you had in the bug report. mport java.awt.BasicStroke; public class AlphaPrintingOffsets implements Printable {
}
} |
@vtstydev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close (!) I see just a couple of minor things I need you to fix.
@@ -0,0 +1,199 @@ | |||
/* | |||
* Copyright (c) 2007, 2022, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you copied the header including "2007, 2022," from some other file.
This is a new test so it should just be "2024,"
import javax.print.attribute.PrintRequestAttributeSet; | ||
import javax.print.attribute.standard.Sides; | ||
import javax.swing.*; | ||
import java.awt.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use wildcard imports - the example I pasted had none !
So please explicitly list all imports and we "sort" these. generally alphabetically at least for the same module.
So the order of the packages should be
java.awt
java.awt.event
java.awt.print
javax.print
javax.swing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@prsadhuk please be the required 2nd reviewer.
@vtstydev 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:
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 739 new commits pushed to the
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 (@prrace, @prsadhuk, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@@ -2418,12 +2419,12 @@ protected int printPage(Pageable document, int pageIndex) | |||
* We also need to translate by the adjusted amount | |||
* so that printing appears in the correct place. | |||
*/ | |||
int bandX = deviceLeft - deviceAddressableX; | |||
int bandX = deviceLeft; | |||
if (bandX < 0) { | |||
bandGraphics.translate(bandX/xScale,0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we test this path where another translate is being done on the same object after it is done in l2399?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet works when application sets negative start point of imageable area of paper.
I have tested it using
paper.setImageableArea(-50, -50, paper.getWidth(), paper.getHeight())
LANDSCAPE oriented and REVERSE_LANDSCAPE oriented images printed with transparency aren't the same as the corresponded images printed with opacity. They are shifted in the opposite direction.
I suggest nobody tested printing with negative start point before. It's strange when start point is situated in the negative area in my opinion. Is there a sense to assume it may be used in such way?
@@ -2396,6 +2396,7 @@ protected int printPage(Pageable document, int pageIndex) | |||
* the page on the next iteration of the loop. | |||
*/ | |||
bandGraphics.setTransform(uniformTransform); | |||
bandGraphics.translate(-deviceAddressableX,deviceAddressableY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a is needed between 2 arguments as per coding guidelines..
} else { | ||
System.out.println("Printer not configured or available." | ||
+ " Test cannot continue."); | ||
PassFailJFrame.forcePass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is now being preferred to throw jtreg.SkippedException instead of forcePass for printers not available scenarios?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrintGlyphVectorTest, ClippedImages use PassFailJFrame.forcePass();
I guided these samples.
If you want I to to use throw
istead. ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess those are old tests which need to be migrated to the new SkippedException semantics...
java/awt/print/PrinterJob/PageRangesDlgTest.java is relatively newer migrated test..and since it is mentioned in the test that it needs physical printer to reproduce and PDF printer will not do, then no point making it pass if physical printer is not present, that is my opinion
Also, if it affects only windows platform, will it not be more advisable to do in WPrinterJob class instead of RasterPrinterJob? |
Revert the changes to the test which limit the pages printed out. |
@prrace |
I mean by default, when run by a testing framework, the test should print all those pages. PS those options also complicate the instructions to the user ! |
@prrace I would like to backport this path to jdk8. May I ask you to add labels into JBS ? |
import javax.print.attribute.standard.Sides; | ||
import javax.swing.JFrame; | ||
import jtreg.SkippedException; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this but the test should FAIL if there is no printer.
SkippedException is appropriate if there were no way to filter printer tests which REQUIRE a printer to do anything useful but there IS, and it is "@key printer".
import static java.awt.print.PageFormat.LANDSCAPE; | ||
import static java.awt.print.PageFormat.PORTRAIT; | ||
import static java.awt.print.PageFormat.REVERSE_LANDSCAPE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static imports always go after regular ones.
// other imports
import javax.swing.JFrame;
import static java.awt.print.PageFormat.LANDSCAPE;
import static java.awt.print.PageFormat.PORTRAIT;
import static java.awt.print.PageFormat.REVERSE_LANDSCAPE;
public class AlphaPrintingOffsets { | ||
private static final String INSTRUCTIONS = | ||
"Tested bug occurs only on-paper printing so you mustn't use PDF printer\n\n" + | ||
"1.Java print dialog should appear.\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"1.Java print dialog should appear.\n" + | |
"1. Java print dialog should appear.\n" + |
"3. Please check the page margin rectangle are properly drawn and visible on all sides on all pages.\n" + | ||
"If so, press PASS, else press FAIL.\n\n" + | ||
"Also you may run this test in paper-saving mode. Due to tested bug affects pages printed with transparency in LANDSCAPE and REVERSE_LANDSCAPE orientations " + | ||
"there is an option to print only 2 pages affected. To do it pass PaperSavingMode parameter."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are longer than 80-column limit, they're longer than 120-column.
You can also reduce the indentation of the string, usually such lines are indented by 8 spaces, so that the opening quote would align with ‘s’ in static
keyword.
} | ||
} | ||
|
||
class CustomPrintable implements Printable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making CustomPrintable
a static nested class in AlphaPrintingOffsets
.
It avoids conflicts when you mark a folder as test sources in an IDE; if another test has a top-level class with the same name, CustomPrintable
, compilation of test sources fails because of duplicate class.
It does not affect jtreg though, so it's just a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many tests in test/jdk/java/awt/print/PrinterJob/ where side classes are not nested. I am badly familiar with nuances of project. You know better how it be more convenient. I will do it if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the reason why I find it inconvenient. Therefore I'd rather not add add another test case which could make it worse.
Yet there are no strict rules that prescribe one way over the other. I'm not insisting.
private static final int THICKNESS = 10; | ||
private static final int MARGIN = 15; | ||
private static final int SMALL_RECTANGLE_SIZE = 5; | ||
private int alphaValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private int alphaValue; | |
private final int alphaValue; |
The value of alphaValue
doesn't change after it's set in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year should be updated to 2024?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright must contain two years at most, you modify the second one to be the current year. With the three years, it will break the build because of incorrect copyright header.
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved. | |||
* Copyright (c) 1998, 2023, 2024, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Copyright (c) 1998, 2023, 2024, Oracle and/or its affiliates. All rights reserved. | |
* Copyright (c) 1998, 2024, Oracle and/or its affiliates. All rights reserved. |
"Tested bug occurs only on-paper printing " + | ||
"so you mustn't use PDF printer\n\n" + | ||
"1. Java print dialog should appear.\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Tested bug occurs only on-paper printing " + | |
"so you mustn't use PDF printer\n\n" + | |
"1. Java print dialog should appear.\n" + | |
"Tested bug occurs only on-paper printing " + | |
"so you mustn't use PDF printer\n\n" + | |
"1. Java print dialog should appear.\n" + |
Alternatively, reduce the indentation of all the lines with the instructions.
System.out.println("Test failed : Printer not configured or available."); | ||
throw new RuntimeException("Test failed : Printer not configured or available."); | ||
System.out.println("Test failed : " + | ||
"Printer not configured or available."); | ||
throw new RuntimeException("Test failed : " + | ||
"Printer not configured or available."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep those unmodified, the string is not so long, and wrapping it doesn't improve readability.
In my opinion, println
is redundant, the runtime exception conveys it.
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
else { | |
} else { |
This is the Java style and you follow this style in other places.
PassFailJFrame.builder().instructions(instructionsHeader + INSTRUCTIONS) | ||
.rows(15).testUI(() -> createTestUI()).build().awaitAndCheck(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PassFailJFrame.builder().instructions(instructionsHeader + INSTRUCTIONS) | |
.rows(15).testUI(() -> createTestUI()).build().awaitAndCheck(); | |
PassFailJFrame.builder() | |
.instructions(instructionsHeader + INSTRUCTIONS) | |
.rows(15) | |
.testUI(() -> createTestUI()) | |
.build() | |
.awaitAndCheck(); |
What do you think? Is it easier to read?
Looks like we are down to a few formatting things. Hopefully we can get this closed out very soon. |
@vtstydev guess you can do "\integrate" for us to "sponsor" |
Or rather, @vtstydev, issue the |
/integrate |
/sponsor |
Going to push as commit 56c5084.
Your commit was automatically rebased without conflicts. |
@aivanov-jdk @vtstydev Pushed as commit 56c5084. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
More correct way to take in consideration nonzero PHYSICALOFFSETX, PHYSICALOFFSETY of device for banded-raster printing loop. Only on Windows platform under certain conditions real device prints shifted image on paper.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17030/head:pull/17030
$ git checkout pull/17030
Update a local copy of the PR:
$ git checkout pull/17030
$ git pull https://git.openjdk.org/jdk.git pull/17030/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17030
View PR using the GUI difftool:
$ git pr show -t 17030
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17030.diff
Webrev
Link to Webrev Comment