<AWT Dev> [9] Review request for 8044444 The output's 'Page-n' footer does not show completely.
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Mon Apr 20 14:08:44 UTC 2015
Hello,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8044444/webrev.04
On 4/17/2015 2:03 AM, Phil Race wrote:
> 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 RasterPrinterJob.attributes were null in case printDialog()
with no-args was used before the regression so attributeToPageFormat()
was not called.
I added additional attributes.isEmpty() check to the
getPageFormatFromAttributes() to preserve this behavior.
>
>
> 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.
The test is updated.
Thanks,
Alexandr.
>
> 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