<AWT Dev> [9] Review request for 8044444 The output's 'Page-n' footer does not show completely.
Phil Race
philip.race at oracle.com
Thu Apr 16 23:03:12 UTC 2015
It looks a bit odd that the new methods in RPJ.java don't use the
PageFormat parameter since its needed only in the subclass over-ride ..
But at least the shared code logic remains the same.
Did you look into this earlier comment :-
>I am not entirely sure that in the case you use printDialog() with
no-args
>and print() with no-args that we really should be in this
attributesToPageFormat()
>method at all.
The test still has this problem I mentioned :-
>
> In the test 'custom' case I see you use printDialog(attributeset) but
> then print()
> The normal pattern is to use it consistently so that the changes made
> by the
> user in the attributeset are propagated.
150
151 boolean printAccepted = job.printDialog(printAttributes);
152 if (printAccepted) {
153 try {
154 job.print();
-phil.
On 3/12/15 8:47 AM, Alexander Scherbatiy wrote:
>
> Hello,
>
> Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8044444/webrev.03
>
>
> On 12/4/2014 4:17 AM, Phil Race wrote:
>> This looks like it might help in so far as it implies that the
>> bug was an incorrect guess at what should be the imageable area
>> But I have a lot of uncertainties that I need to investigate.
>>
>> Eg when I try your test case with 8u20 I don't see a vertical margin
>> of anywhere near 1 inch so if we are getting margins from the code
>> you over-rode, why is that ? Is JTable deliberately drawing outside
>> the imageable area for its header and footer ?
> I tried it with earlier JDK and I see the same behavior. At least
> it is not a regression.
>>
>> Can we just return with defaultPage ? Not if there are updated margins
>> from somewhere else.
> Margins can be set by a user.
>>
>> I am not entirely sure that in the case you use printDialog() with
>> no-args
>> and print() with no-args that we really should be in this
>> attributesToPageFormat()
>> method at all. Perhaps we can fix it up when we are here but can we
>> avoid
>> this ?
> printLoop method from CPrinterJob.m file has the special
> workaround for JTable.print that leads the attributeToPageFormat()
> method is called:
> // <rdar://problem/4367998> JTable.print attributes are ignored
> jobject pageable = JNFCallObjectMethod(env, jthis,
> jm_getPageable); // AWT_THREADING Safe (!appKit)
> javaPrinterJobToNSPrintInfo(env, jthis, pageable, printInfo);
>
>>
>> Regardless of that there are couple of things that look like bugs in
>> the code :-
>>
>> The PageFormat imageable area takes into account the rotation of the
>> page,
>> the Paper does not. So when you set it like this :-
>>
>> 747 paper.setImageableArea(page.getImageableX(),
>> page.getImageableY(),
>> 748 page.getImageableWidth(),
>> page.getImageableHeight());
>> 749 }
>>
>> .. you are ignoring the potential for LANDSCAPE - or REVERSE_LANDSCAPE.
>>
> I updated it to use the paper imageable area.
>> And what if the default page is based on some large size like A3
>> and then the application has specified a media of A5, but no media
>> printable area ?
>> It appears the code will then try to set the paper's imageable area
>> much larger than the entire paper.
>> Should you not in fact limit it to the size of the paper ?
>> Or arguably limit it to the hardware limited imageable area ?
> It is how it worked before the regression. It can be considered as
> a separated issue.
>
> Thanks,
> Alexandr.
>>
>> In the test 'custom' case I see you use printDialog(attributeset) but
>> then print()
>> The normal pattern is to use it consistently so that the changes made
>> by the
>> user in the attributeset are propagated.
>>
>> On the mac the native 'print' dialog doesn't - so far as I can see -
>> allow you
>> to change the paper size and layout. This is a bit different than
>> other platforms.
>> I guess your bug manifests in the case where this is defaults so it
>> probably
>> doesn't matter.
>>
>> -phil.
>>
>> On 12/03/2014 06:08 AM, Alexander Scherbatiy wrote:
>>> On 12/1/2014 8:28 PM, Phil Race wrote:
>>>> Hmm .. it looks as if this breaks the case when the Swing dialog is
>>>> used, doesn't it ?
>>>>
>>>> I think this update needs to be accompanied by regression tests
>>>> that show that
>>>> this kind of page set up using native & swing dialogs both work.
>>>> We can't easily use the JCK tests for this.
>>>
>>> Could you review the updated fix:
>>> http://cr.openjdk.java.net/~alexsch/8044444/webrev.02
>>>
>>> - The native imageable area is set to the page if it is not
>>> defined in the set of attributes for the printer job.
>>> - The manual test that checks printing with/without print dialog
>>> and with/without media printable area properties is added.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>>>
>>>> -phil.
>>>>
>>>> On 11/27/14 7:45 AM, Alexander Scherbatiy wrote:
>>>>> On 11/21/2014 9:20 PM, Phil Race wrote:
>>>>>> This seems to me to be asking about something I covered already.
>>>>>>
>>>>>> >The latter one appears 'correct' in this case since applying it
>>>>>> second
>>>>>> >fixes the output but I don't have enough information to know why
>>>>>> the
>>>>>> >values differ.
>>>>>>
>>>>>> But you have the test case and I don't ..
>>>>>>
>>>>>> Did you try any of what I suggested ?
>>>>>
>>>>> The CPrinterJob.getPageFormat() returns right selected printer
>>>>> format. The problem is that
>>>>> RasterPrinterJob.attributeToPageFormat() method creates
>>>>> a default page with right page format and overrides page
>>>>> size/imageable area after that to predefined ones.
>>>>> This happens only because the CPrinterJob.printLoop() native
>>>>> method calls javaPrinterJobToNSPrintInfo(...) method after
>>>>> javaPageFormatToNSPrintInfo(...).
>>>>>
>>>>> The JCK has a set of JTable print tests. I run them using the
>>>>> predefined page size/imageable area and with the selected printer
>>>>> settings.
>>>>> The page number is properly printed when the selected printer
>>>>> settings are used.
>>>>>
>>>>> I have updated the fix to preserve the selected printer page size:
>>>>> http://cr.openjdk.java.net/~alexsch/8044444/webrev.01/
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>
>>>>>>
>>>>>> -phil.
>>>>>>
>>>>>> On 11/18/14 8:17 AM, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Phil,
>>>>>>>
>>>>>>> Before the 8011069 fix
>>>>>>> RasterPrinterJob.getPageFormatFromAttributes() method returns
>>>>>>> null for null attributes
>>>>>>> and native page size for ImageableArea has been used.
>>>>>>> After the 8011069 fix the attributes are not null and
>>>>>>> updateAttributesWithPageFormat() method rewrites
>>>>>>> the ImageableArea size to the default constants.
>>>>>>>
>>>>>>> The question is which ImageableArea size is correct? If there
>>>>>>> should be used default values then the 8044444 is not an issue
>>>>>>> as all works as expected.
>>>>>>> If it is necessary to use native size then I can update the
>>>>>>> fix to do that.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>
>>>>>>> On 10/30/2014 11:10 PM, Phil Race wrote:
>>>>>>>> When we reach this code everything in the job is already
>>>>>>>> configured by a
>>>>>>>> combination of initial settings and user updates and and we
>>>>>>>> just need to read
>>>>>>>> the settings and pass it on to the native NSPrintInfo.
>>>>>>>> So surely switching the order should not matter unless one of
>>>>>>>> these
>>>>>>>> is using the 'wrong' PageFormat ?
>>>>>>>>
>>>>>>>> -----------------
>>>>>>>> the body of the method called here :-
>>>>>>>>
>>>>>>>> javaPrinterJobToNSPrintInfo(env, jthis, pageable, printInfo);
>>>>>>>>
>>>>>>>> gets its PageFormat as follows :-
>>>>>>>>
>>>>>>>> static JNF_MEMBER_CACHE(jm_getPageFormat, sjc_CPrinterJob,
>>>>>>>> "getPageFormatFromAttributes", "()Ljava/awt/print/PageFormat;");
>>>>>>>> ....
>>>>>>>>
>>>>>>>> jobject page = JNFCallObjectMethod(env, srcPrinterJob,
>>>>>>>> jm_getPageFormat);
>>>>>>>> if (page != NULL) {
>>>>>>>> javaPageFormatToNSPrintInfo(env, NULL, page, dst);
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> So this uses the result of making a call to
>>>>>>>> RasterPrinterJob.getPageFormatFromAttributes()
>>>>>>>>
>>>>>>>> protected PageFormat getPageFormatFromAttributes() {
>>>>>>>> if (attributes == null) {
>>>>>>>> return null;
>>>>>>>> }
>>>>>>>> return attributeToPageFormat(getPrintService(),
>>>>>>>> this.attributes);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -----------------------------
>>>>>>>>
>>>>>>>> whereas
>>>>>>>>
>>>>>>>> javaPageFormatToNSPrintInfo(env, jthis, page, printInfo);
>>>>>>>>
>>>>>>>> is using a PageFormat obtained as follows :-
>>>>>>>>
>>>>>>>> static JNF_MEMBER_CACHE(jm_getPageFormat, sjc_CPrinterJob,
>>>>>>>> "getPageFormat", "(I)Ljava/awt/print/PageFormat;");
>>>>>>>>
>>>>>>>> jobject page = JNFCallObjectMethod(env, jthis,
>>>>>>>> jm_getPageFormat, 0);
>>>>>>>>
>>>>>>>> This is CPrinterJob.getPageFormat() { .. }
>>>>>>>>
>>>>>>>>
>>>>>>>> Although its not easily apparent what the returned values are
>>>>>>>> in each of these cases
>>>>>>>> it does seem they must be different.
>>>>>>>>
>>>>>>>> The latter one appears 'correct' in this case since applying it
>>>>>>>> second
>>>>>>>> fixes the output but I don't have enough information to know
>>>>>>>> why the
>>>>>>>> values differ.
>>>>>>>>
>>>>>>>> Looking at the fix for 8011069, it avoided an NPE by always
>>>>>>>> creating an 'attributes' map, albeit an empty one.
>>>>>>>> This can change the result of calling
>>>>>>>> getPageFormatFromAttributes from
>>>>>>>> 'null' to a PageFormat built from an empty attribute set.
>>>>>>>> If the no-args native printDialog() and the no-args print()
>>>>>>>> call is used this will be empty.
>>>>>>>>
>>>>>>>> So the method will indeed build - at that moment - a page
>>>>>>>> format built from
>>>>>>>> default values.
>>>>>>>>
>>>>>>>> Now. If we *do* use the printDialog(PrintRequestAttributeSet) and
>>>>>>>> print(PrintRequestAttributeSet) methods, then it may well be
>>>>>>>> that this
>>>>>>>> method is the one that should be called.
>>>>>>>>
>>>>>>>> And I think we were previously only in this block of code if
>>>>>>>> that were the case
>>>>>>>> by virtue of the block being guarded by "if (page != NULL)",
>>>>>>>> which means
>>>>>>>> there is an attributeset, which previously meant one of those
>>>>>>>> "with args"
>>>>>>>> methods had been used.
>>>>>>>>
>>>>>>>> So I wonder/suspect if the switching of the order will
>>>>>>>> introduce the equivalent
>>>>>>>> problem in that 'with args' case.
>>>>>>>>
>>>>>>>> As you can tell just looking at the webrev its nigh on
>>>>>>>> impossible to tell
>>>>>>>> for sure and you'd probably need to play around with testing
>>>>>>>> changing
>>>>>>>> paper size and orientation in native and cross-platform dialogs
>>>>>>>> to test it.
>>>>>>>>
>>>>>>>> You could start by seeing if the test 'passes' simply by
>>>>>>>> switching to
>>>>>>>> 'with args' before & after your fix - ensuring that the same
>>>>>>>> paper sizes
>>>>>>>> are being used. I am not sure what the default settings were
>>>>>>>> that were
>>>>>>>> created for the empty attribute set vs the ones that are used
>>>>>>>> when you
>>>>>>>> fixed this. You'll have to tell me that.
>>>>>>>>
>>>>>>>> Perhaps what is needed is a unified call to get the PageFormat
>>>>>>>> which
>>>>>>>> can figure out whether to use the attributes or not. And you could
>>>>>>>> check if the call to CPrinterJob.getPageFormat() already
>>>>>>>> performs that ..
>>>>>>>>
>>>>>>>> -phil.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/28/2014 01:03 PM, Alexander Scherbatiy wrote:
>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Could you review the fix?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>> On 7/15/2014 3:28 PM, Alexander Scherbatiy wrote:
>>>>>>>>>>
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Could you review the fix:
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8044444
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~alexsch/8044444/webrev.00
>>>>>>>>>>
>>>>>>>>>> Native method printLoop from CPrinterJob calls
>>>>>>>>>> javaPrinterJobToNSPrintInfo(...) method after
>>>>>>>>>> javaPageFormatToNSPrintInfo(...).
>>>>>>>>>> Both methods set the page size. The initial page size is
>>>>>>>>>> set in defaultPage(PageFormat) method.
>>>>>>>>>> After the fix 8011069 the printDialog() initializes
>>>>>>>>>> attributes which leads that new page size is created in the
>>>>>>>>>> attributeToPageFormat(PrintService, PrintRequestAttributeSet)
>>>>>>>>>> method.
>>>>>>>>>>
>>>>>>>>>> The fix changes order of the
>>>>>>>>>> javaPrinterJobToNSPrintInfo(...)
>>>>>>>>>> javaPageFormatToNSPrintInfo(...) call so initial page size is
>>>>>>>>>> set at the end.
>>>>>>>>>>
>>>>>>>>>> There is the JCK test that covers the issue.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Alexandr.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the awt-dev
mailing list