-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8314070: javax.print: Support IPP output-bin attribute extension #16166
8314070: javax.print: Support IPP output-bin attribute extension #16166
Conversation
👋 Welcome back alexsch! A progress list of the required criteria for merging this PR into |
@AlexanderScherbatiy The following label will be automatically applied to this pull request:
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. |
Webrevs
|
Why did you file a new RFE when you know about the existing one ? |
* <p> | ||
* <b>IPP Compatibility:</b> The category name returned by {@code getName()} is | ||
* the IPP attribute name. The {@code toString()} method returns the IPP | ||
* string representation of the attribute value. |
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 think this needs to mention that it is an IPP extension attribute, meaning not a standard one
For example like here
https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/javax/print/attribute/standard/PresentationDirection.html
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 javadoc is updated to
* <b>IPP Compatibility:</b> This attribute is not an IPP 1.1 attribute; it is
* an attribute in the "output-bin" attribute extension
* (<a href="https://ftp.pwg.org/pub/pwg/candidates/cs-ippoutputbin10-20010207-5100.2.pdf">
* PDF</a>) of IPP 1.1. The category name returned by {@code getName()} is the
* IPP attribute name. The enumeration's integer value is the IPP enum value.
* The {@code toString()} method returns the IPP string representation of the
* attribute value.
and @Override
annotations are added to OutputBin
and CustomOutputBin
methods.
My idea was to use JDK-8314070 as an umbrella and provide fixes for macOS, Linux, and Windows separately. While the OutputBin public API is not fixed any change in it would require to re-test the fix on all 3 platforms. |
I'm struggling to follow the logic. If you really want to stagger it then proceed as follows
|
@AlexanderScherbatiy 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! |
This is some investigation which I did for output bin attribute extension support on Linux. PostScript Language Reference manual third edition defines The example of
This is my implementation of adding
I tested it with Kyocera ECOSYS M8130cidn printer using Ubuntu 20.04.1 LTS.
The result was that a page is printed to the same output tray (Top) no matter First, I checked that printing a pdf file from the Unix PDF Viewer app using the system print dialog and choosing different output trays really prints pages to different output trays. Second, I copied post script files sent by java program for printing from
which
the setting for output bin is The file which sends the PDF Viewer for printing includes It is not clear for me why One more question which I have relates to the java common print dialog. It needs to contain list of available output bins. Is it safe to assume that the first output bin retrieved by |
@AlexanderScherbatiy this pull request can not be integrated into git checkout JDK-8318023
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
/issue add 8314070 |
@AlexanderScherbatiy |
@AlexanderScherbatiy 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 It has been tested on Linux, MacOS, and Windows with several installed printers. |
@prrace @prsadhuk Actually the native dialog test works on MacOS only if the native print dialog includes Finishing Options with the list of output bins. May be it is better to split the |
@@ -46,6 +46,7 @@ | |||
import javax.print.attribute.standard.MediaSizeName; | |||
import javax.print.attribute.standard.PageRanges; | |||
import javax.print.attribute.standard.Sides; | |||
import javax.print.attribute.standard.OutputBin; |
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.
Please sort the import...Guess OutputBin should come before PageRanges
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.
Updated.
@@ -90,6 +90,7 @@ | |||
import javax.print.attribute.standard.RequestingUserName; | |||
import javax.print.attribute.standard.SheetCollate; | |||
import javax.print.attribute.standard.Sides; | |||
import javax.print.attribute.standard.OutputBin; |
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.
Sort the import..OutputBin should be placed before PageRanges
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.
Updated.
outputBinAttr = (OutputBin)attributes.get(OutputBin.class); | ||
if (!isSupportedValue(outputBinAttr, attributes)) { | ||
outputBinAttr = null; | ||
} |
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.
Should we set it to null if user-set output-bin attribute is not among supported values
or
should we set it to default
as is done for printer-resolution and others
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.
It seems that setting the outputBinAttr to null should be fine as it means that no OutputBin value is provided by jdk and the printer will use the default one.
cbOutput.setEnabled(outputEnabled); | ||
lblOutput.setEnabled(outputEnabled); | ||
|
||
cbOutput.addItemListener(this); |
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.
If Attribute category is unsupported, the panel will be disabled, in that case, is there any need to add itemlistener? I guess we can make it conditional to category being supported
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 fix is updated to add the itemListener only when the cbOutput JComboBox is enabled.
REAR, | ||
FACE_UP, | ||
FACE_DOWN, | ||
LARGE_CAPACITY, |
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.
What about this support?
@@ -1643,7 +1649,9 @@ private String[] printExecCmd(String printer, String options, | |||
execCmd[n++] = "-o job-sheets=standard"; | |||
} | |||
if ((pFlags & OPTIONS) != 0) { | |||
execCmd[n++] = "-o" + options; | |||
for(String option: options.trim().split(" ")) { |
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.
code formatting..space between for and (
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.
code formatting is updated.
@@ -1666,7 +1674,9 @@ private String[] printExecCmd(String printer, String options, | |||
execCmd[n++] = "-o job-sheets=standard"; | |||
} | |||
if ((pFlags & OPTIONS) != 0) { | |||
execCmd[n++] = "-o" + options; | |||
for(String option: options.trim().split(" ")) { |
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.
space is needed for coding style guideline between for and {
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.
updated.
import javax.print.attribute.standard.Media; | ||
import javax.print.attribute.standard.MediaSizeName; | ||
import javax.print.attribute.standard.MediaSize; | ||
import javax.print.attribute.standard.MediaTray; | ||
import javax.print.attribute.standard.OutputBin; |
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.
Import sorting improper...Should be after MediaPrintableArea and before PrinterResolution
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 OutputBin import is updated.
|
||
/* | ||
* @test | ||
* @bug JDK-8314070 |
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.
should be just bugid
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.
JDK-
prefix is removed.
|
||
/* | ||
* @test | ||
* @bug JDK-8314070 |
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.
only bugid is needed
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.
updated.
|
||
/* | ||
* @test | ||
* @bug JDK-8314070 |
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.
same here only bugid needed
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.
updated.
REAR, | ||
FACE_UP, | ||
FACE_DOWN, | ||
LARGE_CAPACITY, |
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.
What I understood is, it is acceptable to identify or get
tray-1, tray-2, tray-3 name
instead of descriptive names like
TOP, MIDDLE, BOTTOM respectively etc
so as such it can support upto 11 trays ie FROM TOP till LARGE_CAPACITY
but am not sure if it is to be added in addition or in-place of the descriptive names?
/* | ||
* @test | ||
* @bug 8314070 | ||
* @key printer |
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.
DO you want to run this on all platforms? Other tests are for linux and mac only!! but guess it's not a problem..
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.
Yes, this test is intended to run on all platforms. On Windows it just checks that there are no unexpected exceptions if OutputBin
category is passed to PrintService
methods.
The |
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.
Overall Looks good to me although I could not test it fully to my liking owing to non-support of output-bin in my printer..
@prrace |
Academic now, since I just approved it. |
/integrate |
Going to push as commit c7d2a5c.
Your commit was automatically rebased without conflicts. |
@AlexanderScherbatiy Pushed as commit c7d2a5c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The fix adds new public
OutputBin
print attribute class which allow to set a printer output bin in aPrinterJob
class. The corresponding internalCustomOutputBin
class is added as well.OutputBin
class are based on Internet Printing Protocol (IPP): “output-bin” attribute extension document.CUPSPrinter.getOutputBins(String printer)
method uses PPDppdFindOption(..., "OutputBin")
function to get supported output bins for the given printer on native level.OutputBin
attribute from the printer job attributes toNSPrintInfo
print settings withOutputBin
key on macOS.The fix was tested on
Kyocera ECOSYS M8130cidn
printer whereppdFindOption(..., "OutputBin")
call returns 4 output bins (text, choice):if
Printer settings
,Inner tray
, orFinisher (face-down)
CustomOutputBins is set toPrinterJob.print(...)
attributes a test page is printed to the Main tray of theKyocera ECOSYS M8130cidn
printer. IfSeparator tray
is used a page is printed to the Separator tray. This is consistent with the printer behavior when a native print dialog is used from a native Preview app to print a document on macOS.Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16166/head:pull/16166
$ git checkout pull/16166
Update a local copy of the PR:
$ git checkout pull/16166
$ git pull https://git.openjdk.org/jdk.git pull/16166/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16166
View PR using the GUI difftool:
$ git pr show -t 16166
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16166.diff
Webrev
Link to Webrev Comment