[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-awt-dev
Subject: Re: <AWT Dev> [9] Review request for 8044444 The output's 'Page-n' footer does not show completely.
From: Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date: 2015-04-28 12:27:04
Message-ID: 553F7C98.9080506 () oracle ! com
[Download RAW message or body]
Hi, Alexander.
Looks fine.
On 20.04.15 17:08, Alexander Scherbatiy wrote:
>
> 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.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--
Best regards, Sergey.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic