<AWT Dev> [9] Review request for 8044444 The output's 'Page-n' footer does not show completely.
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Mar 12 15:47:14 UTC 2015
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