Hi, Here is the updated Webrev with comment fixes:
http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_06/
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_06/>
Thanks and regards,
Shashi
*From:*Shashidhara Veerabhadraiah
*Sent:* Monday, June 12, 2017 11:19 PM
*To:* Philip Race <philip.r...@oracle.com>; Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
*Cc:* 2d-dev@openjdk.java.net
*Subject:* RE: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java needs update by removing a
infinite loop
Ok Phil. I shall update the test and send it for re-review tomorrow.
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Monday, June 12, 2017 11:15 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>; Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com <mailto:prasanta.sadhuk...@oracle.com>>
*Cc:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java needs update by removing a
infinite loop
The dialog does not show "Letter" because you said so. It shows it
because the printer reports it supports letter.
-phil.
On 06/12/2017 10:44 AM, Shashidhara Veerabhadraiah wrote:
Just to clarify Phil for the part of cross platform print dialog,
here is that dialog representation:
Since this dialog can appear on any platform, the Paper class
would represent the backend object for the front end dialog
properties(of a generic printer) as shown in the pic above. Hence
the use of the raw Paper object than obtaining it from the page
format.
At the same time, as you pointed out, the test is not reliable
with the actual conditions. The only problem I see is that the
moment we change the test case to stay close to the actual
scenario we may have certain failures because the settings are
different for different locales and it may be difficult to adapt
to every possible locales.
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Monday, June 12, 2017 10:01 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>; Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com> <mailto:prasanta.sadhuk...@oracle.com>
*Cc:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java needs update by
removing a infinite loop
*/>[Shashi] Yes. I intend to use the default paper object as this is a test
related to the cross platform default printer dialog./*
*/> In the modified test file, I have set the size of the physical paper
instead of relying it on the default setting which may/*
*/> vary as you pointed out, depending on the locale./*
*/>Now that we have our own paper object(with a constant paper size) and
based on the margin setting(which are const/*
*/> it won’t cause an undesirable behavior like going into negative
space./*
Sorry, that is not a valid thing to do. The print dialog is free to ignore
this
or workaround the incompatibility of that paper with the supported media so
your test is not reliable.
And I don't see what the cross platform print dialog has to do with it.
It is, or should be, just as aware of the printer sizes as the native one.
-phil.
On 06/12/2017 03:34 AM, Shashidhara Veerabhadraiah wrote:
Hi Phil, Please see below for the comments:
The updated Webrev is at:
http://cr.openjdk.java.net/~aghaisas/shashi/6949753/webrev_05/
<http://cr.openjdk.java.net/%7Eaghaisas/shashi/6949753/webrev_05/>
Modified in what way from the previous version ?
-phil.
Thanks and regards,
Shashi
*From:*Phil Race
*Sent:* Saturday, June 10, 2017 3:07 AM
*To:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>; Shashidhara
Veerabhadraiah <shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>
*Cc:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java needs update by
removing a infinite loop
private static void setValuesForPrintPageSetup(PageFormat pageFormat,
int
118 marginValue) throws PrinterException {
119 Paper paper = new Paper();
double paperHeight = paper.getHeight();
122 double paperWidth = paper.getWidth();
123 double paperX = paper.getImageableX();
124 double paperY = paper.getImageableY();
125 paper.setImageableArea(paperX * marginValue, paperY *
marginValue,
126 paperWidth - (paperX * 2 * marginValue),
127 paperHeight - (paperY * 2 * marginValue));
105 setValuesForPrintPageSetup(pageFormat, 3);
I see you call new Paper() above
https://docs.oracle.com/javase/8/docs/api/java/awt/print/Paper.html#Paper--
Did you really intend to use a default paper instead of getting the one
from the pageFormat ? On some label printer your Letter Paper may not
even be supported. US (aka NA) Letter is 8.5" wide.
Also although it probably will work out OK the maths isn't checking
for boundary problems.
default margin will be 1" so that's what you'll get for paperX and
paperY
Using your value of 3 we set the imagable area such that
imageable X = 1 * 3 = 3
imageableWidth = 8.5 - (1 * 2 *3) = 8.5 - 6 = 2.5;
Fortunately that worked out positive .. but it does not seem to be
enforced.
If we'd used 5 it would be a different story :
ix = 5, iw = 8.5 - ( 1 * 2 * 5) = -1.5
The implementation will (should) clamp it to non-negative but it
might still be better to have some defensive logic of your own.
*/[Shashi] Yes. I intend to use the default paper object as this is a
test related to the cross platform default printer dialog. In the modified test
file, I have set the size of the physical paper instead of relying it on the
default setting which may vary as you pointed out, depending on the locale./*
*/Now that we have our own paper object(with a constant paper size) and
based on the margin setting(which are constants), it won’t cause an undesirable
behavior like going into negative space./*
nit: there's a missing space here
75 } catch(PrinterException e) {
*/[Shashi] This is fixed now./*
-phil.
On 06/09/2017 03:27 AM, Prasanta Sadhukhan wrote:
looks good to me.
Regards
Prasanta
On 6/9/2017 3:49 PM, Shashidhara Veerabhadraiah wrote:
Hi All, Please find the updated Webrev with fixes for
the comments @
http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_04/
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_04/>
Thanks and regards,
Shashi
*From:*Philip Race
*Sent:* Thursday, June 8, 2017 3:32 AM
*To:* Prasanta Sadhukhan
<prasanta.sadhuk...@oracle.com>
<mailto:prasanta.sadhuk...@oracle.com>
*Cc:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java needs
update by removing a infinite loop
.. and please make sure all lines are <= 80 chars as
per the coding standards.
-phil.
On 6/6/17, 11:59 PM, Prasanta Sadhukhan wrote:
do_test() does not need to be under EDT as it
invokes printer pagedialog and not swing
components. Actually, createUI() needs to be under
EDT which has not been done.
Also,
79 SwingUtilities.invokeAndWait(() -> {
80 test.disposeUI();
81 });
82 }
should be called before you throw RuntimeException when
test times out .
There is no need of calling this after
75 if (test.testResult == false) {
76 throw new RuntimeException("Test
Failed.");
77 }
as it has already been called in pass/fail actionlistener.
Also, put a sleep after T1.start() and do_test() otherwise
since they are in separate thread, in mycase, pagedialog is displayed before
test instructions dialog.
Regards
Prasanta
On 6/7/2017 11:50 AM, Shashidhara Veerabhadraiah
wrote:
Hi All,
I have altered the manual test template per
the comments.
1.Have moved the test instructions window
under newly created thread.
2.Have moved the print dialog(main test
module) under EDT.
3.Timer management shall be done on the main
thread.
I have placed the updated Webrev @
http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_03/
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_03/>
Please let me know if any comments on it.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Tuesday, June 6, 2017 11:52 AM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Cc:* Philip Race <philip.r...@oracle.com>
<mailto:philip.r...@oracle.com>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java
needs update by removing a infinite loop
As I told, pageDialog is modal so
latch.await() will not be called if user does
not close the page dialog or do any
interaction. The actual test
59 PageFormat pageFormat = new PageFormat();
60
61 createNewPrintPageSetup(pageFormat);
62
63 setValuesForPrintPageSetup(pageFormat, 2);
64
65 createNewPrintPageSetup(pageFormat);
66
67 setValuesForPrintPageSetup(pageFormat, 3);
68
69 createNewPrintPageSetup(pageFormat);
should be done in other thread.
Regards
Prasanta
On 6/6/2017 11:24 AM, Shashidhara
Veerabhadraiah wrote:
The manual test template that I received
from the team seems buggy and an older
version it seems. I have modified the same
per your inputs and now placed the updated
Webrev at
http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_02/
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_02/>.
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Monday, June 5, 2017 12:35 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Cc:* Philip Race <philip.r...@oracle.com>
<mailto:philip.r...@oracle.com>
*Subject:* Re: [9]JDK-6949753:[TEST BUG]:
java/awt/print/PageFormat/PDialogTest.java
needs update by removing a infinite loop
I guess there is one more problem in usage
of CountDown latch. Have you seen this
test fail with timeout even if you wait
for 5 minutes as per your timeout period?
latch.await() needs to be wait on main
thread while the test needs to be executed
in another thread otherwise, pageDialog
being modal the control will not come to
latch.await()
Iguess you need to do this.
TestUI test = new TestUI(latch);
Thread T1 = new Thread(test);
T1.start();
class TestUI implements Runnable {
...
@Override
public void run() {
try {
createUI();
Regards
Prasanta
On 6/2/2017 4:00 PM, Shashidhara
Veerabhadraiah wrote:
Hi, I have fixed the comments below
and updated the webrev @
http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_01/
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_01/>
Thanks and regards,
Shashi
*From:*Prasanta Sadhukhan
*Sent:* Friday, June 2, 2017 12:36 PM
*To:* Shashidhara Veerabhadraiah
<shashidhara.veerabhadra...@oracle.com>
<mailto:shashidhara.veerabhadra...@oracle.com>;
2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Cc:* Philip Race
<philip.r...@oracle.com>
<mailto:philip.r...@oracle.com>
*Subject:* Re: [9]JDK-6949753:[TEST
BUG]:
java/awt/print/PageFormat/PDialogTest.java
needs update by removing a infinite loop
Test fix look ok. Only thing is, you
can call getPrinterJob() once and
reutilise instead of calling 3 times
and probably there is no need of
creating a
functioncreateNewPrintPageSetup() for
it (as it calls 1 method) but it is
upto you.
Few comments:
Copyright should have "," after 2017.
I guess createUI() does not have any
call that throws exception so no need
to have try-catch block for createUI().
Also, there is no need to catch
PrinterException and rethrow
RuntimeException, so you can do away
with that try-catch.
Also, you can call disposeUI() in
passButton and failButton
actionlistener instead of in main().
Also, there is no need to do
setVisible(false) in disposeUI(),
dispose() will take care of that.
You can throw RuntimeException when
test timed out (instead of just
println and later getting test fail
exception) which is different from
Test Failed RuntimeException.
Regards
Prasanta
On 6/1/2017 5:10 PM, Shashidhara
Veerabhadraiah wrote:
Hi All,
Please review a fix for a test bug which
contained an infinite loop to test the printer setup dialog's margin attributes
retention without the manual step procedure.
The issue with PDialogTest.java which tests
the printer setup dialog's margin attributes retention by having as infinite
loop to keep popping up the dialog without a proper exit. The test does not
cover the instruction steps necessary to properly test dialog's margin
attributes retention.
The updated test file includes the standard
manual test template along with test cases to cover the printer dialog's margin
attributes retention feature.
Bug:
<https://bugs.openjdk.java.net/browse/JDK-6949753>
Webrev:
<http://cr.openjdk.java.net/~pkbalakr/shashi/6949753/webrev_00/>
<http://cr.openjdk.java.net/%7Epkbalakr/shashi/6949753/webrev_00/>
Note : PrintDialog on Mac does not show
page margins and hence this test does not run on Mac.
Thanks and regards,
Shashi
