Hi Prasanta,
Changes are working fine.
Please add @Override annotation for methods in test case and increase sleep
time little bit more for test case before pushing.
No need for one more review.
Thanks,
Jay
-----Original Message-----
From: Prasanta Sadhukhan
Sent: Monday, August 01, 2016 2:37 PM
To: Phil Race
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-6575247: Banner checkbox in
PrinterJob print dialog doesn't work
On 7/28/2016 9:19 PM, Phil Race wrote:
> right .. when we talked off-line yesterday I said that if IPP is
> reporting a default we should honour it. The default of standard/on is
> only for the case we do not use IPP or it reports nothing.
>
> if (noJobSheet) {
> 885 pFlags |= NOSHEET;
> 886 ncomps+=1;
> 887 } else {
> 888 ncomps+=1;
> 889 }
>
> ....
> 905 if ((pFlags & NOSHEET) != 0) {
> 906 execCmd[n++] = "-o nobanner";
> 907 } else if (getPrintService().
> 908 isAttributeCategorySupported(JobSheets.class)) {
> 909 execCmd[n++] = "-o job-sheets=standard";
> 910 }
>
> So shouldn't the else at line 887 have the same condition as at line
> 907 ?
Yes. Have modified the webrev to add the check
http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.04/
>
> 2750 JobSheets js = (JobSheets)asCurrent.get(jsCategory);
> 2751 if (js == null) {
> 2752 js =
> (JobSheets)psCurrent.getDefaultAttributeValue(jsCategory);
> 2753 if (js == null) {
> 2754 js = JobSheets.STANDARD;
> 2755 }
>
> Please make sure this isn't causing a surprise on OSX or Windows ..
> ie that we report correctly a value there so that "null" doesn't end
> up causing us to print a banner page where we did not before.
>
Tested on osx and windows too where it behaves as it is now ie no banner page
is printed by default.
Regards
Prasanta
> -phil.
>
> On 7/28/2016 2:51 AM, Prasanta Sadhukhan wrote:
>> The banner checkbox is not enabled initially as it calls
>> getDefaultAttributeValue() and it seems printer is reporting the
>> default job-sheet value as "none" so the below check is not satisfied.
>> cbJobSheets.setSelected(js != JobSheets.NONE);
>>
>> When IPPPrintService#readIPPResponse() is called, it invokes /new
>> AttributeClass(attribStr, valTagByte, outArray);
>> /and stored the value corresponding to each attribute.
>>
>> I saw that IPP response has
>> *job-sheets-default => job-sheets-default *and*
>> **job-sheets-supported => job-sheets-supported*
>>
>> When AttributeClass constructor is called with ("job-sheets-default",
>> 66, value) the value is
>> [ 0] = (byte) 4 [ 1] = (byte) 110 [ 2] = (byte) 111 [ 3] =
>> (byte) 110 [ 4] = (byte) 101 [ 5] = (byte) 4 [ 6] = (byte)
>> 110 [ 7] = (byte) 111 [ 8] = (byte) 110 [ 9] = (byte) 101
>> [10] = (byte) 2
>> which basically translates to '',n,o,n,e,'',n,o,n,e,''
>>
>> so when ServiceDialog calls
>> IPPPrintService#getDefaultAttributeValue(JobSheets)
>> ----------------
>> if (attribClass != null &&
>> attribClass.getStringValue().equals("none")) {
>> return JobSheets.NONE;
>> } else {
>> return JobSheets.STANDARD;
>> }
>> --------------
>> we have attribClass.getStringValue() as "none" so default job sheet
>> value is coming out to be NONE.
>> Since it is coming out from IPP response, I think we should not
>> change default value of JobSheets to STANDARD. Do you think otherwise?
>>
>> In the modified webrev, I have made the jobsheet default value
>> STANDARD, only when IPP does not report any default value.
>>
>> http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.03/
>>
>> Tested in ubuntu & solaris11.
>>
>> Regards
>> Prasanta
>> On 7/27/2016 7:20 PM, Philip Race wrote:
>>> 883 } else {
>>> 884 Class<?>[] supportedCats = getPrintService().
>>> 885 getSupportedAttributeCategories();
>>> 886 for (int i=0;i<supportedCats.length;i++) {
>>> 887 if (JobSheets.class == supportedCats[i]) {
>>> 888 pFlags |= JOBSHEET;
>>> 889 ncomps+=1;
>>> 890 break;
>>> 891 }
>>> 892 }
>>>
>>> What is wrong with
>>>
>>> getPrintService().isAttributeCategorySupported(JobSheets.class) ?
>>>
>>> https://docs.oracle.com/javase/8/docs/api/javax/print/PrintService.h
>>> tml#isAttributeCategorySupported-java.lang.Class-
>>>
>>>
>>> I am also very confused about why you added JOBSHEET which seems to
>>> duplicate the functionality of NOSHEET.
>>>
>>> Also it seems to me like it was intentional that the banner page be
>>> printed by default .. which is why the variable was called
>>> "*no*JobSheet ..
>>> so as to over-ride that behaviour.
>>>
>>> It is kind of what you'd want if you walk over to the shared printer
>>> in your office to have a banner page which declares it as yours ...
>>>
>>> So the checkbox should probably be enabled in that case.
>>>
>>> Also you should verify that we report the default value for
>>> JobSheets as being STANDARD, not NONE.
>>>
>>>
>>> -phil.
>>>
>>> On 7/27/16, 3:02 AM, Prasanta Sadhukhan wrote:
>>>> Modified webrev to take care of a problem in webrev.01 whereby
>>>> banner page was getting printed by default.
>>>> Now, banner page will get printed only if the checkbox is checked
>>>> in printer dialog.
>>>>
>>>> http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.02/
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 7/22/2016 4:26 PM, Prasanta Sadhukhan wrote:
>>>>> Hi Phil,
>>>>>
>>>>> I have modified the code to check if job-sheets is supported and
>>>>> then only proceed to print the banner page.
>>>>> Also, rectified the jobTitle and banner confusion by adding
>>>>> jobsheet check.
>>>>> Also added the same code in UnixPrintJob even though I found its
>>>>> printExecCmd() does not get executed in linux and solaris
>>>>> PSPrinterJob's printExecCmd() is executed instead. In mac, neither
>>>>> UnixPrinterJob#printExecCmd() nor PSPrinterJob#printExecCmd() gets
>>>>> executed.
>>>>>
>>>>> Tested on ubuntu and solaris 11 with the fix and banner page is
>>>>> printed with the fix. In mac, cover page gets printed without any
>>>>> change.
>>>>>
>>>>> http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.01/
>>>>>
>>>>> Regards
>>>>> Prasanta
>>>>> On 7/20/2016 8:56 PM, Philip Race wrote:
>>>>>> In my evaluation of that bug (which was 9 yrs ago so I do not
>>>>>> have any memory of it :-)), I note that we first need to check
>>>>>> that job-sheets is supported .. you are not doing that ..
>>>>>> what happens if we pass an unsupported option ?
>>>>>> "-o foobar" ?? This needs testing on Linux, OS X, and Solaris 11.
>>>>>>
>>>>>> Also -J (job-title) is something you can see in the queue when
>>>>>> you do lpq. I don't know why it is tied to banner here but
>>>>>> removing it seems wrong. Perhaps this should be renamed from
>>>>>> "banner" and "BANNER" to jobTitle ?? Please examine this.
>>>>>>
>>>>>> In fact you seem to be conflicting with the -o nobanner.
>>>>>>
>>>>>> So in general we have some lack of clarity around job title and
>>>>>> banner page (aka job sheet).
>>>>>>
>>>>>> Also we have PSPrinterJob and UnixPrinterJob with similar code.
>>>>>> Please examine it to make sure no similar case is going missed.
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 7/18/16, 4:27 AM, Prasanta Sadhukhan wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Please review a fix for an issue where it is seen that Banner
>>>>>>> page in linux does not get printed despite user selecting the
>>>>>>> Banner page checkbox in Printer dialog.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6575247
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~psadhukhan/6575247/webrev.00/
>>>>>>>
>>>>>>> It seems if we pass "-J <bannername>" option to lpr command, it
>>>>>>> has no effect.
>>>>>>> To print Banner page, we need to pass "-o job-sheets=<either of
>>>>>>> "classified", "confidential", "secret", "standard", "topsecret",
>>>>>>> or "unclassified">"
>>>>>>>
>>>>>>> Since we support only standard banner or none via a checkbox,
>>>>>>> the proposed fix passes option "-o job-sheets=standard" to lpr
>>>>>>> command to print the banner page.
>>>>>>>
>>>>>>> Regards
>>>>>>> PRasanta
>>>>>
>>>>
>>
>
