Re: [9] Review request for 8044444 The output's 'Page-n' footer does not show completely.

[Phil Race]

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.