[OpenJDK 2D-Dev] [9] Review request for 8167102: [macosx] PrintRequestAttributeSet breaks page size set using PageFormat

Prasanta Sadhukhan prasanta.sadhukhan at oracle.com
Tue Mar 28 14:23:21 UTC 2017



On 3/28/2017 6:25 PM, Anton Litvinov wrote:
> Hello Prasanta,
>
> The correct "PageFormat" for each separate page is retrieved in the 
> native function "PrinterView.m::rectForPage:(NSInteger)" by invoking 
> the Java method "CPrinterJob.getPageformatPrintablePeekgraphics(int)" 
> with the first expression specified below and getting the value of 
> "PageFormat" from the returned array with the second expression 
> specified below.
>
> "jobjectArray objectArray = JNFCallObjectMethod(env, fPrinterJob, 
> jm_getPageformatPrintablePeekgraphics, jPageNumber); // AWT_THREADING 
> Safe (AWTRunLoopMode)"
> "jobject pageFormat = (*env)->GetObjectArrayElement(env, objectArray, 0);"
>
> But in that native method "PrinterView.m::rectForPage" only the page 
> orientation field "mOrientation" of the retrieved "PageFormat" for 
> each page is analyzed and is set to "NSPrintInfo" object through 
> "NSPrintOperation". Thus for some reason at that method analysis of 
> the paper size is ignored.
>
> So obviously not taking into account individual paper size of the 
> pages for the case, which you described, when the user's code 
> specifies different "PageFormat" instances for different pages of the 
> document, should take place in JDK, though I did not verify this 
> practically. But this issue existed before my fix and is not a result 
> of the fix.
>
> I do not see anything bad in the hard coded "0" page number used in my 
> fix, because we need to initialize "NSPrintInfo" with a valid page 
> size and the page size of the first page of the document is the only 
> correct or appropriate value for this moment, and because this 
> approach already exists in "RasterPrinterJob.java" as the next expression.
>
> "PageFormat pf = (PageFormat)pageable.getPageFormat(0).clone();"
>
> From my point of view, the issue about disrespect of different paper 
> sizes for different pages of the document is the issue which is 
> different from the issue described in this bug (JDK-8167102) and 
> should be fixed separately from my bug, because:
> - In my bug "java.awt.print.Printable" interface is involved, while in 
> this new issue "java.awt.print.Pageable" interface is the explicit 
> requirement;
> - In my bug calling "PrinterJob.print(PrintRequestAttributeSet)" with 
> a non-empty "PrintRequestAttributeSet" is the obligatory and the key 
> requirement, while in the new issue this condition is irrelevant.
> - For my bug one regression test is necessary, while for the new issue 
> the different regression test is necessary.
>
> Therefore I suggest to file a separate bug for the discovered issue 
> and to address it separately from this bug (JDK-8167102). Would you 
> agree with this suggestion? Would you approve the second version of 
> the fix for the bug JDK-8167102?
>
So long the other issue is addressed (even separately), I am ok with 
this fix.

Regards
Prasanta
> Thank you,
> Anton
>
> On 3/28/2017 12:46 PM, Prasanta Sadhukhan wrote:
>>
>> Hi Anton,
>>
>>
>> On 3/27/2017 10:05 PM, Anton Litvinov wrote:
>>> Hello Prasanta,
>>>
>>> Indeed it is Mac specific, as it was written in my previous e-mail, 
>>> I verified this fact on Windows OS and Linux OS by your request.
>>>
>>> Yes, I am fully sure that, when the bug occurs, the paper size of 
>>> the printed page is wrong, as it is stated in the bug, and I am 
>>> fully sure that with the applied any of 2 versions of the created 
>>> fix, the paper size of the printed page becomes correct and as 
>>> expected. Otherwise, I would not send the fix for review. The 
>>> instruction, by following which the bug can be reproduced, is 
>>> written in "STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :" section of 
>>> the description of the bug in JBS. This bug is raised by the user, 
>>> who owns a support contract and requests for resolution of this bug, 
>>> this can be seen in proper comments of the bug record.
>>>
>>> The automated regression test is not possible for this bug, since it 
>>> is necessary to verify visually that the document is physically 
>>> printed on the paper of the expected size. Otherwise, I would send 
>>> the 1st version of the fix with the automated test already, it is 
>>> not the first bug in JDK on which I have been working. By your 
>>> request the regression test, even though it is manual, was created 
>>> and added to the 2nd version of the fix.
>>>
>>> Yes, it is correct, the implemented by the test, and the test case 
>>> method "Printable.print(Graphics, PageFormat, int)" receives the 
>>> correct instance of "PageFormat" in runtime in scenario described in 
>>> this bug, because, as you already described, it is extracted using 
>>> the expression "pageable.getPageFormat(pageIndex)" in the Java 
>>> method 
>>> "sun.lwawt.macosx.CPrinterJob.getPageformatPrintablePeekgraphics(int)" 
>>> called from "PrinterView.m::rectForPage:(NSInteger)" and further 
>>> transferred in this method to the Java method 
>>> "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea".
>>>
>>> The wrong paper size which is returned from the method 
>>> "RasterPrinterJob.getPageFormatFromAttributes()" and which is set to 
>>> certain fields of the Objective-C  object "NSPrintInfo" in the 
>>> function "CPrinterJob.m::javaPaperToNSPrintInfo" through the next 
>>> call sequence
>>>
>>> CPrinterJob.m::printLoop() --> 
>>> CPrinterJob.m::javaPrinterJobToNSPrintInfo() --> 
>>> CPrinterJob.m::javaPageFormatToNSPrintInfo() --> 
>>> CPrinterJob.m::javaPaperToNSPrintInfo()
>>>
>>> further influences physical size of all pages printed by 1 printer job.
>>>
>>> Thus, I consider that firstly "PageFormat" returned from 
>>> "RasterPrinterJob.getPageFormatFromAttributes()" is wrong, and 
>>> secondly setting paper size from this wrong "PageFormat" to proper 
>>> fields of "NSPrintInfo" object is also incorrect and is a root cause 
>>> of this bug.
>> OK. So, pageformat values set to native NSPrintInfo is different 
>> (wrong) compared to what is being passed to user.
>>>
>>> Implementation of Java Print Server API surely contains many issues, 
>>> and only working on this bug I already encountered 2 additional 
>>> different issues, which are described in one of my comments in this 
>>> bug record in JBS. However, I prefer to resolve issues separately 
>>> and to resolve this particular bug also separately from other issues 
>>> which we can indefinitely find while looking at the code and 
>>> debugging it, also because it is necessary to deliver the fix for 
>>> this bug to "jdk8u-dev" repository finally, while JDK 9 is currently 
>>> in RDP 2 phase at which large fixes affecting more platforms or 
>>> large code scope would hardly be accepted by Group and Area leads or 
>>> the release team.
>>>
>>> I would like to bring also your attention again to the fact, which 
>>> was mentioned in my previous e-mail, that I already ran all manual 
>>> and automatic "jtreg" regression tests related to printing from open 
>>> and closed parts of JDK on JDK 9 without the fix and with the fix, 
>>> what is 198 tests.
>>>
>>> Do you find anything incorrect in the 2nd version of the fix? Will 
>>> you approve the 2nd version of the fix?
>>>
>> I think there might be a (probable) issue with this fix. PageFormat 
>> of 1st page only is used to get the information.
>> jobject page = JNFCallObjectMethod(env, srcPrinterJob, 
>> jm_getPageFormat, (jint)0);
>> What if user has set different pageformat to different page like this 
>> below? Will it not lose the 2nd page pageformat? I guess in 
>> windows/linux, we obtain pageformat for each pageindex
>>
>> PrinterJob job = PrinterJob.getPrinterJob();
>> // Create a landscape pageformat
>> PageFormat pfl = job.defaultPage();
>> pfl.setOrientation(PageFormat.LANDSCAPE);
>> //Setup a book
>> Book bk = new Book();
>> bk.append(new xPrintable(), pfl); // 1st page landscape , can be 
>> diff. pagesize too
>> bk.append(new xxPrintable(), job.defaultPage()); //2nd page portrait
>> job.setPageable(bk);
>>
>> Regards
>> Prasanta
>>> Thank you,
>>> Anton
>>>
>>> On 3/27/2017 9:52 AM, Prasanta Sadhukhan wrote:
>>>>
>>>> Hi Anton,
>>>>
>>>> Ok. So, it seems it mac specific. But, are you sure wrong page size 
>>>> is used in mac as is claimed in the bug?
>>>> I thought we could use automated regression test instead of manual 
>>>> by checking pageformat value in print() as compared to what user 
>>>> passes in setPrintable().
>>>>
>>>> Then, I see in print() method in testcase, the "PageFormat" 
>>>> argument passed has same values as it is passed in setPrintable() 
>>>> in main() even without your fix.
>>>>
>>>> If I reverse trace from print() method present in testcase, I see 
>>>> it is called from CPrinterJob.java#printAndGetPageFormatArea()
>>>> which is called from PrinterView.m#rectForPage. And before calling 
>>>> printAndGetPageFormatArea() it calls 
>>>> getPageformatPrintablePeekgraphics() which calls 
>>>> pageable.getPageFormat(pageIndex), so it should behave same as 
>>>> windows. Am I missing something?
>>>>
>>>> Regards
>>>> Prasanta
>>>> On 3/27/2017 3:32 AM, Anton Litvinov wrote:
>>>>> Hello Prasanta,
>>>>>
>>>>> I verified that the bug is not reproducible using JDK 9 compiled 
>>>>> from the latest version of "jdk9/client" forest neither on 
>>>>> Windows, nor on Linux platform, therefore, yes, this bug is only 
>>>>> Mac specific. Debugging showed that on Windows platform the 
>>>>> behavior is exactly like you described, on Windows 
>>>>> "RasterPrinterJob.print(PrintRequestAttributeSet)" method is 
>>>>> responsible for printing the documents and in this method 
>>>>> "RasterPrinterJob.printPage(Pageable, int)" is called for each 
>>>>> page separately, and in this "printPage" method "PageFormat" 
>>>>> instance used for printing of the page is extracted from the the 
>>>>> transferred as the method argument instance of "Pageable" by the 
>>>>> expression "origPage = document.getPageFormat(pageIndex);".
>>>>>
>>>>> The new version of the fix was created. Could you please review 
>>>>> the second version of the fix.
>>>>>
>>>>> Webrev (the 2nd version): 
>>>>> http://cr.openjdk.java.net/~alitvinov/8167102/jdk9/webrev.01
>>>>>
>>>>> In the second version of the fix:
>>>>> 1. Calling for "RasterPrinterJob.getPageFormatFromAttributes()" 
>>>>> was substituted for 
>>>>> "sun.lwawt.macosx.CPrinterJob.getPageFormat(int)" in the native 
>>>>> method "CPrinterJob.m#javaPrinterJobToNSPrintInfo".
>>>>> 2. The method "RasterPrinterJob.getPageFormatFromAttributes()" was 
>>>>> removed, since it is not called from any other code in "jdk" 
>>>>> repository.
>>>>> 3. The manual regression test was created.
>>>>>
>>>>> Also on OS X I executed all manual and automatic "jtreg" 
>>>>> regression tests related to printing from the listed below 
>>>>> directories and the corresponding directories in the closed 
>>>>> repositories using both JDK 9 compiled without and with the fix, 
>>>>> and I verified that no new test failed on JDK 9 with the fix.
>>>>>
>>>>> The directories with the regression tests from open repositories:
>>>>> - "jdk/test/java/awt/print"
>>>>> - "jdk/test/javax/print"
>>>>>
>>>>> Thank you,
>>>>> Anton
>>>>>
>>>>> On 3/22/2017 7:42 AM, Prasanta Sadhukhan wrote:
>>>>>>
>>>>>> Hi Anton,
>>>>>>
>>>>>> I thought about your point and have a question: does this issue 
>>>>>> not reproduce in non-mac platform?
>>>>>> I thought it probably would so suggested modifying 
>>>>>> setAttributes() . But, if it is only reproduced in mac, we need 
>>>>>> to find out why despite this setAttributes() flaw, how this is 
>>>>>> working in non-mac platform?
>>>>>>
>>>>>> It probably might work in windows/linux because in 
>>>>>> RasterPrinterJob.print(attr) method, after it calls 
>>>>>> setAttributes(), it calls printPage() where it gets the original 
>>>>>> PageFormat
>>>>>> by calling getPageFormat(pageIndex) and gets the orientation, 
>>>>>> imageablearea
>>>>>> and not by constructing pageFormat from attributes as it is done 
>>>>>> in mac.
>>>>>> So, probably, in mac also, we need to do away with 
>>>>>> getPageFormatFromAttributes() call  and call 
>>>>>> getPageFormat(pageIndex) from 
>>>>>> CPrinterJob.m#javaPrinterJobToNSPrintInfo().
>>>>>>
>>>>>> Regards
>>>>>> Prasanta
>>>>>> On 3/21/2017 8:15 PM, Anton Litvinov wrote:
>>>>>>> Hello Prasanta,
>>>>>>>
>>>>>>> Thank you for review of this fix. I have tried to apply the 
>>>>>>> approach which you suggested and it allowed to resolve the 
>>>>>>> issue. In this case I agree that it would be more correct to 
>>>>>>> resolve the issue in 
>>>>>>> "RasterPrinterJob.setAttributes(PrintRequestAttributeSet)" 
>>>>>>> method. In the first version of the fix I decided to change the 
>>>>>>> method "RasterPrinterJob.getPageFormatFromAttributes()", because 
>>>>>>> it is invoked only from 1 place in JDK code and this place is 
>>>>>>> located in OS X specific code 
>>>>>>> "jdk/src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m", 
>>>>>>> so that would automatically minimize the affected by the fix 
>>>>>>> platforms to OS X only.
>>>>>>>
>>>>>>> Starting to work on implementation of the second version of the 
>>>>>>> fix including the regression test.
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Anton
>>>>>>>
>>>>>>> On 3/21/2017 12:52 PM, Prasanta Sadhukhan wrote:
>>>>>>>>
>>>>>>>> I think the real problem is not in 
>>>>>>>> RasterPrinterJob.getPageFormatFromAttributes().
>>>>>>>>
>>>>>>>> One can see that, when we call RasterPrinterJob.setPrintable(), 
>>>>>>>> we call updatePageAttributes() which in turn calls 
>>>>>>>> updateAttributesWithPageFormat() where orientation, media and 
>>>>>>>> mediaprintablearea "attributes" get populated/cached from the 
>>>>>>>> "pageformat" supplied by the user.
>>>>>>>>
>>>>>>>> But, when PrinterJob.print(PrintRequestAttributeSet) is called, 
>>>>>>>> it calls setAttributes(attributes) where "attributes" is what 
>>>>>>>> is populated by the user. It does not contain orientation,media 
>>>>>>>> and mediaprintablearea attributes for this bug, so 
>>>>>>>> setAttributes sets cached attribute with this user-set 
>>>>>>>> attribute instance
>>>>>>>> />>else {//
>>>>>>>> //>>            // for AWT where pageable is not an instance of 
>>>>>>>> OpenBook,//
>>>>>>>> //>>            // we need to save paper info//
>>>>>>>> // >>           this.attributes = attributes;//
>>>>>>>> // >>       }//
>>>>>>>> /
>>>>>>>>
>>>>>>>> //thereby losing the attributes it has cached earlier from user 
>>>>>>>> pageformat. I think we should check if this.attributes is not 
>>>>>>>> null, then retain those attributes unless explicitly set by the 
>>>>>>>> user in user-defined attributes.
>>>>>>>>
>>>>>>>> But, this code is used by windows,linux,mac so it needs to be 
>>>>>>>> tested against all those platforms to ensure we are not 
>>>>>>>> regressing anything.
>>>>>>>>
>>>>>>>> Also, I think user should call validatePage(PageFormat) in 
>>>>>>>> application to check if the settings passed is compatible with 
>>>>>>>> the printer or not, get compatible/adjusted pageformat and call 
>>>>>>>> setPrintable() with that "adjusted" pageformat. If user pass 
>>>>>>>> 0,0,fullpaperwidth,fullpaperheight as imageablearea, then he 
>>>>>>>> should not expect this setting to be used since printer will 
>>>>>>>> have its own hardware limitation (and use some offset printing)
>>>>>>>>
>>>>>>>> BTW, a regression testcase (even if it is manual) should be 
>>>>>>>> created with the fix.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Prasanta
>>>>>>>> On 3/17/2017 10:59 PM, Anton Litvinov wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Could you please review the following fix for the bug.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8167102
>>>>>>>>> Webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~alitvinov/8167102/jdk9/webrev.00
>>>>>>>>>
>>>>>>>>> The bug consists in the fact that, if the user's application 
>>>>>>>>> specifies the required page size (media size) through 
>>>>>>>>> "java.awt.print.PrinterJob" API particularly via 
>>>>>>>>> "java.awt.print.PageFormat" instance supplied to the method 
>>>>>>>>> "PrinterJob.setPrintable(Printable, PageFormat)" and calls 
>>>>>>>>> "PrinterJob.print(PrintRequestAttributeSet)" with 
>>>>>>>>> "javax.print.attribute.PrintRequestAttributeSet" containing at 
>>>>>>>>> least 1 any "PrintRequestAttribute", then the page or pages 
>>>>>>>>> will be printed with "PageFormat" describing the default page 
>>>>>>>>> size of the printer which is different from the expected and 
>>>>>>>>> originally set by the user's application "PageFormat".
>>>>>>>>>
>>>>>>>>> Debugging showed that the new "PageFormat" with unexpected 
>>>>>>>>> media size is created in the method 
>>>>>>>>> "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" 
>>>>>>>>> which is invoked from the native function 
>>>>>>>>> "jdk/src/java.desktop/macosx/native/libawt_lwawt/awt/CPrinterJob.m::javaPrinterJobToNSPrintInfo". 
>>>>>>>>> The method "RasterPrinterJob.getPageFormatFromAttributes()" 
>>>>>>>>> returns a new "PageFormat" always, if the provided by the user 
>>>>>>>>> "PrintRequestAttributeSet" is not empty, and the default 
>>>>>>>>> values are set to particular instance variables of this 
>>>>>>>>> "PageFormat", if "PrintRequestAttributeSet" does not contain 
>>>>>>>>> the searched "OrientationRequested", "MediaSizeName", 
>>>>>>>>> "MediaPrintableArea" attributes.
>>>>>>>>>
>>>>>>>>> THE SOLUTION:
>>>>>>>>> Although it is clearly stated in Java Platform SE 8 API 
>>>>>>>>> Specification (documentation of the method 
>>>>>>>>> "PrinterJob.print(PrintRequestAttributeSet)")
>>>>>>>>> Specification URL: 
>>>>>>>>> http://docs.oracle.com/javase/8/docs/api/java/awt/print/PrinterJob.html#print-javax.print.attribute.PrintRequestAttributeSet-
>>>>>>>>>
>>>>>>>>> that media size, orientation and imageable area attributes 
>>>>>>>>> specified in "PrintRequestAttributeSet" supplied to the method 
>>>>>>>>> "PrinterJob.print" will be used for construction of a new 
>>>>>>>>> "PageFormat", which will be passed to "Printable" object, 
>>>>>>>>> instead of the original "PageFormat" instance set through 
>>>>>>>>> "PrinterJob.setPrintable" method, the observed in this issue 
>>>>>>>>> behavior is a bug, because in the bug test case neither media 
>>>>>>>>> size, nor orientation, nor imageable area attributes are 
>>>>>>>>> specified in "PrintRequestAttributeSet".
>>>>>>>>>
>>>>>>>>> The fix alters the method 
>>>>>>>>> "sun.print.RasterPrinterJob.getPageFormatFromAttributes()" to 
>>>>>>>>> use corresponding values from "PageFormat" instance originally 
>>>>>>>>> set through "PrinterJob" API during construction of the new 
>>>>>>>>> "PageFormat", when it is found out that 
>>>>>>>>> "OrientationRequested", or "MediaSizeName", or 
>>>>>>>>> "MediaPrintableArea" attribute is not specified in 
>>>>>>>>> "PrintRequestAttributeSet" supplied to "PrinterJob.print" method.
>>>>>>>>>
>>>>>>>>> Thank you,
>>>>>>>>> Anton
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170328/ac6df9f6/attachment.html>


More information about the 2d-dev mailing list