jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8314070: javax.print: Support IPP output-bin attribute extension

Open AlexanderScherbatiy opened this issue 2 years ago • 32 comments

The fix adds new public OutputBin print attribute class which allow to set a printer output bin in a PrinterJob class. The corresponding internal CustomOutputBin class is added as well.

  • Constants used in OutputBin class are based on Internet Printing Protocol (IPP): “output-bin” attribute extension document.
  • CUPSPrinter.getOutputBins(String printer) method uses PPD ppdFindOption(..., "OutputBin") function to get supported output bins for the given printer on native level.
  • The fix propagates the OutputBin attribute from the printer job attributes to NSPrintInfo print settings with OutputBin key on macOS.

The fix was tested on Kyocera ECOSYS M8130cidn printer where ppdFindOption(..., "OutputBin") call returns 4 output bins (text, choice):

  • Printer settings, None
  • Inner tray, INNERTRAY
  • Separator tray, SEPARATORTRAY
  • Finisher (face-down), Main

if Printer settings, Inner tray, or Finisher (face-down) CustomOutputBins is set to PrinterJob.print(...) attributes a test page is printed to the Main tray of the Kyocera ECOSYS M8130cidn printer. If Separator 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

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [x] Change requires CSR request JDK-8331601 to be approved

Warning

 ⚠️ Found leading lowercase letter in issue title for 8314070: javax.print: Support IPP output-bin attribute extension

Issues

  • JDK-8314070: javax.print: Support IPP output-bin attribute extension (Enhancement - P4)
  • JDK-8331601: javax.print: Support IPP output-bin attribute extension (CSR)

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

AlexanderScherbatiy avatar Oct 12 '23 15:10 AlexanderScherbatiy

:wave: Welcome back alexsch! 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.

bridgekeeper[bot] avatar Oct 12 '23 15:10 bridgekeeper[bot]

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

openjdk[bot] avatar Oct 12 '23 15:10 openjdk[bot]

Why did you file a new RFE when you know about the existing one ? Close your new one as a dup. and redirect everything to the existing one. Also I don't see any UI work to enhance the dialog so it can be selected. And why restrict it to macOS ? Is it an issue of testing ?

prrace avatar Oct 17 '23 20:10 prrace

Why did you file a new RFE when you know about the existing one ? Close your new one as a dup. and redirect everything to the existing one. Also I don't see any UI work to enhance the dialog so it can be selected. And why restrict it to macOS ? Is it an issue of testing ?

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. The fix for the UI dialog requires an additional test with a printer dialog so there would be 2 tests for each platform for re-testing for each essential change in the fix.

AlexanderScherbatiy avatar Oct 18 '23 17:10 AlexanderScherbatiy

Why did you file a new RFE when you know about the existing one ? Close your new one as a dup. and redirect everything to the existing one. Also I don't see any UI work to enhance the dialog so it can be selected. And why restrict it to macOS ? Is it an issue of testing ?

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. The fix for the UI dialog requires an additional test with a printer dialog so there would be 2 tests for each platform for re-testing for each essential change in the fix.

I'm struggling to follow the logic. This change clearly contains the public API which is the first thing you need. Burying it in the fix for one platform that happens to be the first one you coded isn't right.

If you really want to stagger it then proceed as follows

  1. Add just the new API class under the existing RFE
  2. Add this macOS support for it under this ID
  3. Add the Swing dialog UI support for selecting the bin - easier once you have one implementation
  4. Add Linux support (CUPS, so should be similar to macOS)
  5. Add Windows support, but SFAIK GDI has no support for this, so likely impossible to do.

prrace avatar Nov 02 '23 21:11 prrace

@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!

bridgekeeper[bot] avatar Dec 01 '23 00:12 bridgekeeper[bot]

This is some investigation which I did for output bin attribute extension support on Linux.

PostScript Language Reference manual third edition defines OutputType page device parameter with type string which represents special attributes of the output destination.

The example of OutputType usage I have found in CUPS PPD Extensions. OutputType specifies the output type name, for example:

<</OutputType (Photo)>>setpagedevice

This is my implementation of adding OutputType parameter in PSPrinterJob https://github.com/AlexanderScherbatiy/jdk/commit/92ce5b9e23e0338c0d1b49a653dbc67e0512bdc4 added to a separate branch

    protected void startDoc() throws PrinterException {
            ...
            String outputBin = getOutputBinValue(outputBinAttr);

            if (outputBin != null) {
                mPSStream.print(" /OutputType (" + outputBin + ") ");
            }

I tested it with Kyocera ECOSYS M8130cidn printer using Ubuntu 20.04.1 LTS. The ppdFindOption(ppd, "OutputBin") function from CUPSfuncs returns two output bins for the Kyocera ECOSYS M8130cidn:

text: Top, choice: Top
text: Stacker 1, choice: Stacker1

The result was that a page is printed to the same output tray (Top) no matter Top or Stacker1 output bin is used for OutputType post script parameter.

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 /var/spool/cups directory. The post script file for Top output bin contains:

%!PS-Adobe-3.0
...
%%BeginSetup
<< /PageSize [595.2755737304688 841.8897705078125] /DeferredMediaSelection true /ImagingBBox null /ManualFeed false /NumCopies 1 /OutputType (Top)  >> setpagedevice
%%EndSetup

which /OutputType (Top) setting looks correct. And for the Stacker1 output bin:

%%BeginSetup
<< /PageSize [595.2755737304688 841.8897705078125] /DeferredMediaSelection true /ImagingBBox null /ManualFeed false /NumCopies 1 /OutputType (Stacker1)  >> setpagedevice
%%EndSetup

the setting for output bin is /OutputType (Stacker1).

The file which sends the PDF Viewer for printing includes %PDF-1.5 version and I was not able to find the parameter corresponding to the selected output bin even if I convert the file from pdf to ps format using pdf2ps tool which generates file in %!PS-Adobe-3.0 fromat.

It is not clear for me why /OutputType (Stacker1) parameter does not work and is there a way to take the output bin parameter which Linux native printer dialog use to print a document to the Stacker 1 tray for the Kyocera ECOSYS M8130cidn printer.

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 ppdFindOption(ppd, "OutputBin") is the default one? Or should there be added one more Default option for the default output bin selection?

AlexanderScherbatiy avatar Dec 07 '23 12:12 AlexanderScherbatiy

@AlexanderScherbatiy this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

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

openjdk[bot] avatar Dec 07 '23 12:12 openjdk[bot]

/issue add 8314070

AlexanderScherbatiy avatar Dec 07 '23 12:12 AlexanderScherbatiy

@AlexanderScherbatiy Adding additional issue to issue list: 8314070: javax.print: Support IPP output-bin attribute extension.

openjdk[bot] avatar Dec 07 '23 12:12 openjdk[bot]

The common print dialog is updated to show supported output bins.

  • OutputPanel is added to the ServiceDialog to show list of supported output bins in the ComboBox.
  • order.output and label.outputbins properties are added to serviceui.properties.
  • CPrinterJob.nsPrintInfoToJavaPrinterJob() method is updated to add the selected output bin from the native print dialog to the printer job attributes.
  • OutputBinAttributePrintDialogTest manual test that checks only printing to the first and the last supported output bins with common and native print dialog is added.

Common print dialog with the fix which shows list of supported output bins on macOS:

The same dialog with disabled output trays when the list of output bins is empty:

The same dialog without the fix:

There is the comment on social.msdn.microsoft.com forum that wingdi does not support setting output bin attribute programmatically:

"'Output tray" is a printer-specific property (note that you will only see such an option
in the printing interface for certain printers).  There is no way to programmatically set 
an output tray, since there is no generic way of doing it via .NET (or even Win32 
via the DEVMODE structure).  The only way to set it is to allow the user
to choose it via the provided printer dialog for the few printers that provide this capability
(just as you must do via IE or Word).

It is sometimes possible to do this programmatically (without the dialog) for a specific
printer if you know exactly how that printer driver implements the change...but that
is unsupported  except possibly from the printer manufacturer.

https://social.msdn.microsoft.com/Forums/en-US/b15eda2f-5353-41fc-b935-078ed0960aee/printing-into-a-specific-output-bintray-net

That is why OutputPanel is excluded from the ServiceDialog on Windows.

AlexanderScherbatiy avatar Dec 13 '23 16:12 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!

bridgekeeper[bot] avatar Jan 10 '24 17:01 bridgekeeper[bot]

@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!

AlexanderScherbatiy avatar Feb 02 '24 17:02 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!

bridgekeeper[bot] avatar Mar 01 '24 21:03 bridgekeeper[bot]

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

8314070: javax.print: Support IPP output-bin attribute extension

Reviewed-by: psadhukhan, 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 439 new commits pushed to the master branch:

  • 6dac8d64527b4e9ade783b99f82fbecd81c426a6: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16
  • 9686e804a2b058955ff88149c54a0a7896c0a2eb: 8333103: Re-examine the console provider loading
  • 4de620732f03c71fec3e1c233947742d334c88ad: 8333229: Parallel: Rename ParMarkBitMap::_region_start to _heap_start
  • 1f9e62904c624b12bd344d2ef3021eb5d3377197: 8333434: IGV: Print loop node for PHASE_BEFORE/AFTER_CLOOPS
  • 27af19d921a5cf15f5146471b58961815690b4f2: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
  • 1c514b34c0260823e70f209996ac933a76ac34c2: 8325435: [macos] Menu or JPopupMenu not closed when main window is resized
  • d07e530d33360dae687552a6dfbe26408f3fb58e: 8333128: Linux x86_32 configure fail with --with-hsdis=binutils --with-binutils-src
  • f0bffbce35bb06e724857e8651dd429c4f9df284: 8333301: Remove static builds using --enable-static-build
  • b10158624bd0cfe009f0fe7f2a06ee08e654766b: 8332514: Allow class space size to be larger than 3GB
  • 5ed0d52c8424dd2e7f1ac2404e9fabb40c8402b8: 8332936: Test vmTestbase/metaspace/gc/watermark_70_80/TestDescription.java fails with no GC's recorded
  • ... and 429 more: https://git.openjdk.org/jdk/compare/b96b38c2c9a310d5fe49b2eda3e113a71223c7d1...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

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!

AlexanderScherbatiy avatar Mar 30 '24 18:03 AlexanderScherbatiy

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 ppdFindOption(ppd, "OutputBin") is the default one? Or should there be added one more Default option for the default output bin selection?

You should definitely query for the default. But then how to use it ..

The way the dialog works is that there's an initial set of attributes (needed if the user cancels) and the "asCurrent" which starts out as a copy of that inital set. Anything not in the initial set, or if the default value is not supported, we look for the default value - assuming that the category is supported at all - and that's what is the selected item in the dialog UI but that does NOT cause the default to be copied to asCurrent. So if the user selects a different printer, the dialog gets the new printer's default - if any is supported. The general idea being anything they didn't touch should mean "use the default".

But if the user explicitly selects a value it is added to asCurrent and carried over to the next printer, so long as it is a supported value there, otherwise its discarded (at least that's the intended idea). So if you don't know the default, it isn't very easy to do the right thing.

prrace avatar Apr 14 '24 00:04 prrace

The updated fix adds the default output bin value on the print dialog as the latest empty line which is selected if the output bin has not been provided as the asCurrent. It is not clear for me is it ok to leave this value as empty or it should have name like Default. In later case it needs to translate the name to other languages as well.

AlexanderScherbatiy avatar May 01 '24 19:05 AlexanderScherbatiy

This is getting close enough that it is time to start on the CSR, since I think the API part is settled now.

prrace avatar May 02 '24 18:05 prrace

/csr

prrace avatar May 02 '24 18:05 prrace

/Reviewers 2

prrace avatar May 02 '24 18:05 prrace

@prrace has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@AlexanderScherbatiy please create a CSR request for issue JDK-8314070 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk[bot] avatar May 02 '24 18:05 openjdk[bot]

I have created the CSR https://bugs.openjdk.org/browse/JDK-8331601 and assigned it to you to fill in. Once you've done that I can review it.

prrace avatar May 02 '24 19:05 prrace

Note that RDP1 for JDK 23 is 6/6/24 - https://openjdk.org/projects/jdk/23/ so there's still time to get it into JDK 23, so you long as you get the CSR submitted soon.

prrace avatar May 04 '24 18:05 prrace

I added the OutputBin class and package-info.java changes to the CSR javax.print: Support IPP output-bin attribute extension description: https://bugs.openjdk.org/browse/JDK-8331601

The OutputBin class has two myStringTable and myEnumValueTable tables. They are private but their values are accessed through the public API. Should myStringTable and myEnumValueTable tables be mentioned in the CSR?

Should the changes in the common print dialog (the output bin label and the output bin combo-box) and new properties from the serviceui.properties be described in the CSR?

AlexanderScherbatiy avatar May 06 '24 18:05 AlexanderScherbatiy

Mailing list message from Philip Race on client-libs-dev:

On 5/6/24 11:08 AM, Alexander Scherbatiy wrote:

On Thu, 2 May 2024 18:30:23 GMT, Alexander Scherbatiy <alexsch at openjdk.org> wrote:

The fix adds new public `OutputBin` print attribute class which allow to set a printer output bin in a `PrinterJob` class. The corresponding internal `CustomOutputBin` class is added as well.

- Constants used in `OutputBin` class are based on [Internet Printing Protocol (IPP): ?output-bin? attribute extension](https://ftp.pwg.org/pub/pwg/candidates/cs-ippoutputbin10-20010207-5100.2.pdf) document. - `CUPSPrinter.getOutputBins(String printer)` method uses PPD `ppdFindOption(..., "OutputBin")` function to get supported output bins for the given printer on native level. - The fix propagates the `OutputBin` attribute from the printer job attributes to `NSPrintInfo` print settings with `OutputBin` key on macOS.

The fix was tested on `Kyocera ECOSYS M8130cidn` printer where `ppdFindOption(..., "OutputBin")` call returns 4 output bins (text, choice): - Printer settings, None - Inner tray, INNERTRAY - Separator tray, SEPARATORTRAY - Finisher (face-down), Main

if `Printer settings`, `Inner tray`, or `Finisher (face-down)` CustomOutputBins is set to `PrinterJob.print(...)` attributes a test page is printed to the Main tray of the `Kyocera ECOSYS M8130cidn` printer. If `Separator 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. Alexander Scherbatiy has updated the pull request incrementally with one additional commit since the last revision:

Change 'Page Setup' to 'Appearance' tab in the OutputBinAttributePrintDialogTest I added the `OutputBin` class and package-info.java changes to the CSR `javax.print: Support IPP output-bin attribute extension` description: https://bugs.openjdk.org/browse/JDK-8331601

The package-info text isn't spec. relevant, so I removed it from the CSR.

The `OutputBin` class has two `myStringTable` and `myEnumValueTable` tables. They are private but their values are accessed through the public API. Should `myStringTable` and `myEnumValueTable` tables be mentioned in the CSR?

I didn't think it was necessary, so I edited this out. In general I cut out anything that wasn't spec and wasn't too hard to edit away.

Should the changes in the common print dialog (the output bin label and the output bin combo-box) and new properties from the `serviceui.properties` be described in the CSR?

No need that I can see.

Good that you got it in and finalized this week.

-phil

mlbridge[bot] avatar May 07 '24 22:05 mlbridge[bot]

@AlexanderScherbatiy did you see the comment in the CSR about sealed classes ? I had originally suggested in this PR (search for a comment from a while ago) and I don't know if you saw it there either.

prrace avatar May 08 '24 15:05 prrace

@AlexanderScherbatiy did you see the comment in the CSR about sealed classes ? I had originally suggested in this PR (search for a comment from a while ago) and I don't know if you saw it there either.

@prrace There are two things which are not clear for me.

The first one is that a user can extend OutputBin class and provide its own implementation. Should it be possible to the user provide output bins other than supported ones?

The another question relates to CustomOutputBin class which is used to provide a list of supported output bins and which is not public. To make OutputBin class sealed it needs to add CustomOutputBin class to OutputBin class permits list:

public sealed class OutputBin extends EnumSyntax implements PrintRequestAttribute, PrintJobAttribute 
    permits sun.print.CustomOutputBin {
}

public final class CustomOutputBin extends OutputBin {
}

Is it good to use a non public class in the public class permits list?

I made docs for the jdk with sealed OutputBin class. It has the sealed keyword in the javadoc but mentions nothing about internal CustomOutputBin class. images/docs/api/java.desktop/javax/print/attribute/standard/OutputBin.html:

public sealed class OutputBin
extends EnumSyntax 
implements PrintRequestAttribute, PrintJobAttribute

AlexanderScherbatiy avatar May 11 '24 13:05 AlexanderScherbatiy

@prrace

I have updated the fix to make the OutputBin class sealed and CustomOutputBin class final. The OutputBin class permits internal sun.print.CustomOutputBin class. The OutputBin javadoc that Implementation- or site-defined names for an output bin kind attribute may also be created by defining a subclass of class {@code OutputBin}. is removed. If it is an applicable change I will update the CSR as well.

AlexanderScherbatiy avatar May 11 '24 17:05 AlexanderScherbatiy