[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