[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-7064425: PageFormat Dialog has no owner window to reactivate
From: Jayathirth D V <jayathirth.d.v () oracle ! com>
Date: 2016-09-13 7:58:06
Message-ID: ffea46cc-4ca6-4c11-8f4c-93295b3b5ecf () default
[Download RAW message or body]
Hi Prasanta,
Changes are working fine.
In both test cases you are not using "private static Label dialogName;". Please \
remove it. Also some lines are more than 80 characters length and test case has \
trailing spaces. Please rectify these things before pushing. No need for one more \
review.
Thanks,
Jay
From: Philip Race
Sent: Saturday, September 10, 2016 12:32 AM
To: Prasanta Sadhukhan
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-7064425: PageFormat Dialog has no owner \
window to reactivate
Sorry, right that won't work .. this is an expression evaluation and the compiler \
needs to determine the return type which is going to end up back as Window as the \
most-specific super-type. Details in 15.25.3 \
http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.25 So it won't \
help us make this code neater. I guess we'll just live with it.
+1
-phil.
On 9/1/16, 2:16 AM, Prasanta Sadhukhan wrote:
Hi Phil,
Please find the modified webrev
HYPERLINK "http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.02/"http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.02/
This takes care of removing DialogOwner attribute before printDialog returns.
However, I could not use the same optimisation in WPrinterJob as mentioned
>>WPrintDialog dialog = new WPrintDialog(((owner instanceof Frame) ? (Frame)owner \
: (Dialog)owner), this)
because it caused compilation failure saying
[no suitable constructor found for WPrintDialog((owner ins[...]owner,WPrinterJob)
WPrintDialog dialog = new WPrintDialog(
^
constructor WPrintDialog.WPrintDialog(Frame,PrinterJob) is not applicable
(argument mismatch; bad type in conditional expression
Dialog cannot be converted to Frame)
constructor WPrintDialog.WPrintDialog(Dialog,PrinterJob) is not applicable
(argument mismatch; bad type in conditional expression
Frame cannot be converted to Dialog)]
I tried a sample program too to see if it's java issue and there also the compilation \
fails class TestA {
public TestA(Dialog temp) {}
public TestA(Frame temp) {}
}
public class TestB {
public static void main(String[] args) {
Window val = new Frame();
TestA test = new TestA((val instanceof Frame) ? (Frame) val : \
(Dialog) val); }
}
It seems compiler needs to determine what constructor to call. So both operands of \
the conditional operator should be assignable to the type of the parameter.
Regards
Prasanta
On 9/1/2016 12:07 AM, Phil Race wrote:
911 public boolean printDialog(final PrintRequestAttributeSet attributes)
...
956 attributes.add(new DialogOwner((Frame)w));
So this now adds the DialogOwner to the attribute set passed by the application.
This needs to be guaranteed to be removed again before the method returns otherwise
apart from leaving that visible to the application, there is some risk that the
frame will not be GC'd when it should have been.
- Frame ownerFrame = (dlgOwner != null) ? dlgOwner.getOwner() : null;
+ Window ownerFrame = (dlgOwner != null) ? dlgOwner.getOwner() : null
so maybe change the var name to just "owner" ?
WPrintDialog dialog;
484 if (ownerFrame instanceof Frame) {
485 dialog = new WPrintDialog((Frame)ownerFrame, this);
486 } else {
487 dialog = new WPrintDialog((Dialog)ownerFrame, this);
488 }
could (should?) be
WPrintDialog dialog =
new WPrintDialog(((owner instanceof Frame) ? (Frame)owner : (Dialog)owner), \
this)
and so on for the other cases too .. it will save a lot
of repetition for new PrintToFileErrorDialog(..)
-phil.
On 08/26/2016 01:05 AM, Prasanta Sadhukhan wrote:
Hi All,
I have modified the webrev to take care of HYPERLINK \
"https://bugs.openjdk.java.net/browse/JDK-6948907"JDK-6948907: sun.print.DialogOwner \
does not support Dialogs as DialogOwner also.
HYPERLINK "http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.01/"http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.01/
Tested on windows and ubuntu.
Regards
Prasanta
On 8/25/2016 4:10 PM, Prasanta Sadhukhan wrote:
Hi All,
Please review a fix for jdk9 for an issue where it is seen that PageDialog and \
PrintDialog is not associated with the owner Frame that spawns the dialog.
Bug: https://bugs.openjdk.java.net/browse/JDK-7064425
webrev: HYPERLINK "http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.00/"http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.00/ \
The issue was there we explicitly pass null as owner to ServiceDialog in \
pageDialog(attributes). Proposed fix is to get the owner window,
if pageDialog is called before calling printDialog, the window will be a Frame else \
the owner window will be ServiceDialog and pass this owner window to ServiceDialog \
instead of null.
For PrintDialog, the proposed fix is to set an attribute with DialogOwner so that \
ServiceUI dialog can parse that attribute and can use the owner window as parent of \
the dialaog,
Regards
Prasanta
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=us-ascii"><meta name=Generator content="Microsoft Word 15 \
(filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.changed
{mso-style-name:changed;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.removed
{mso-style-name:removed;}
span.new
{mso-style-name:new;}
span.EmailStyle22
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body bgcolor=white lang=EN-US link=blue \
vlink=purple><div class=WordSection1><p class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'>Hi \
Prasanta,<o:p></o:p></span></p><p class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'><o:p> </o:p></span></p><p \
class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'>Changes are \
working fine.<o:p></o:p></span></p><p class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'>In both test \
cases you are not using “private static Label dialogName;”. Please remove \
it. Also some lines are more than 80 characters length and test case has trailing \
spaces. Please rectify these things before pushing. No need for one more \
review.<o:p></o:p></span></p><p class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'><o:p> </o:p></span></p><p \
class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'>Thanks,<o:p></o:p></span></p><p \
class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'>Jay \
<o:p></o:p></span></p><p class=MsoNormal><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D'><o:p> </o:p></span></p><div><div \
style='border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext'>From:</span></b><span \
style='font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext'> Philip \
Race <br><b>Sent:</b> Saturday, September 10, 2016 12:32 AM<br><b>To:</b> Prasanta \
Sadhukhan<br><b>Cc:</b> 2d-dev<br><b>Subject:</b> Re: [OpenJDK 2D-Dev] [9] RFR \
JDK-7064425: PageFormat Dialog has no owner window to \
reactivate<o:p></o:p></span></p></div></div><p \
class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Sorry, right that won't work \
.. this is an expression evaluation and the compiler needs to<br>determine the return \
type which is going to end up back as Window as the most-specific \
super-type.<br>Details in 15.25.3 <a \
href="http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.25">http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.25</a><br>So \
it won't help us make this code neater. I guess we'll just live with \
it.<br><br>+1<br><br>-phil.<br><br>On 9/1/16, 2:16 AM, Prasanta Sadhukhan wrote: \
<o:p></o:p></p><blockquote style='margin-top:5.0pt;margin-bottom:5.0pt'><p \
class=MsoNormal>Hi Phil,<br><br>Please find the modified webrev<br><a \
href="http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.02/">http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.02/</a><br><br>This \
takes care of removing DialogOwner attribute before printDialog \
returns.<br><br>However, I could not use the same optimisation in WPrinterJob as \
mentioned<br> >><span class=changed><i>WPrintDialog dialog = \
new WPrintDialog(((owner instanceof Frame)</i></span><i> ? (Frame)owner : \
(Dialog)owner), this)<br></i><br>because it caused compilation failure saying \
<br><i><br>[no suitable constructor found for WPrintDialog((owner \
ins[...]owner,WPrinterJob)<br> WPrintDialog \
dialog = new WPrintDialog(<br> &n \
bsp; \
^<br> constructor WPrintDialog.WPrintDialog(Frame,PrinterJob) is \
not applicable<br> (argument mismatch; bad type in \
conditional expression<br> \
Dialog cannot be converted to Frame)<br> constructor \
WPrintDialog.WPrintDialog(Dialog,PrinterJob) is not \
applicable<br> (argument mismatch; bad type in \
conditional expression<br> \
Frame cannot be converted to Dialog)]<br></i><br>I tried a sample program too to see \
if it's java issue and there also the compilation fails<br>class TestA \
{<br> public TestA(Dialog temp) \
{}<br> public TestA(Frame temp) \
{}<br>}<br>public class TestB {<br> public \
static void main(String[] args) \
{<br> \
Window val = new Frame();<br> \
TestA test = new TestA((val instanceof Frame) ? (Frame) val : (Dialog) \
val);<br> }<br>}<br>It seems compiler needs \
to determine what constructor to call. So both operands of the conditional operator \
should be assignable to the type of the \
parameter.<br><br>Regards<br>Prasanta<o:p></o:p></p><div><p class=MsoNormal>On \
9/1/2016 12:07 AM, Phil Race wrote:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><div><pre> 911 \
public boolean printDialog(final PrintRequestAttributeSet \
attributes)<o:p></o:p></pre><pre>...<o:p></o:p></pre><pre> \
956 \
attributes.add(new \
DialogOwner((Frame)w));<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>So this now \
adds the DialogOwner to the attribute set passed by the \
application.<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>This needs to be \
guaranteed to be removed again before the method returns otherwise \
<o:p></o:p></pre><pre>apart from leaving that visible to the application, there is \
some risk that the<o:p></o:p></pre><pre>frame will not be GC'd when it should have \
been.<o:p></o:p></pre><p class=MsoNormal><o:p> </o:p></p><pre><span \
class=removed>- Frame ownerFrame = \
(dlgOwner != null) ? dlgOwner.getOwner() : null;</span><o:p></o:p></pre><pre><span \
class=new>+ Window ownerFrame = (dlgOwner \
!= null) ? dlgOwner.getOwner() : null<o:p></o:p></span></pre><pre><span \
class=new><o:p> </o:p></span></pre><pre><span class=new>so maybe change the var \
name to just "owner" ?<o:p></o:p></span></pre><pre><span \
class=changed>WPrintDialog dialog;</span><o:p></o:p></pre><pre><span class=changed> \
484 if (ownerFrame instanceof Frame) \
{</span><o:p></o:p></pre><pre><span class=changed> \
485 dialog = \
new WPrintDialog((Frame)ownerFrame, this);</span><o:p></o:p></pre><pre><span \
class=changed> 486 } else \
{</span><o:p></o:p></pre><pre><span class=changed> \
487 dialog = \
new WPrintDialog((Dialog)ownerFrame, this);</span><o:p></o:p></pre><pre><span \
class=changed> 488 \
}<o:p></o:p></span></pre><pre><span \
class=changed><o:p> </o:p></span></pre><pre><span class=changed>could (should?) \
be<o:p></o:p></span></pre><pre><span \
class=changed><o:p> </o:p></span></pre><pre><span \
class=changed><o:p> </o:p></span></pre><pre><span class=changed>WPrintDialog \
dialog =<o:p></o:p></span></pre><pre><span \
class=changed> new WPrintDialog(((owner instanceof \
Frame)</span> ? (Frame)owner : (Dialog)owner), \
this)<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>and so on for the other cases \
too .. it will save a lot<o:p></o:p></pre><pre>of repetition for new <span \
class=new>PrintToFileErrorDialog</span>(..)<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>-phil. \
<o:p></o:p></pre><p class=MsoNormal><br>On 08/26/2016 01:05 AM, Prasanta Sadhukhan \
wrote:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal>Hi All,<br><br>I have \
modified the webrev to take care of <a \
href="https://bugs.openjdk.java.net/browse/JDK-6948907" id=key-val>JDK-6948907</a>: \
sun.print.DialogOwner does not support Dialogs as DialogOwner<br>also.<br><a \
href="http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.01/">http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.01/</a><br><br>Tested \
on windows and ubuntu.<br><br>Regards<br>Prasanta<o:p></o:p></p><div><p \
class=MsoNormal>On 8/25/2016 4:10 PM, Prasanta Sadhukhan \
wrote:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><p class=MsoNormal>Hi All, \
<br><br>Please review a fix for jdk9 for an issue where it is seen that PageDialog \
and PrintDialog is not associated with the owner Frame that spawns the dialog. \
<br><br>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-7064425">https://bugs.openjdk.java.net/browse/JDK-7064425</a> \
<br>webrev: <a href="http://cr.openjdk.java.net/%7Epsadhukhan/7064425/webrev.00/">http://cr.openjdk.java.net/~psadhukhan/7064425/webrev.00/</a> \
<br><br>The issue was there we explicitly pass null as owner to ServiceDialog in \
pageDialog(attributes). <br>Proposed fix is to get the owner window, <br>if \
pageDialog is called before calling printDialog, the window will be a Frame else the \
owner window will be ServiceDialog <br>and pass this owner window to ServiceDialog \
instead of null. <br><br>For PrintDialog, the proposed fix is to set an attribute \
with DialogOwner so that ServiceUI dialog can parse that attribute and can use the \
owner window as parent of the dialaog, <br><br>Regards <br>Prasanta \
<o:p></o:p></p></blockquote><p class=MsoNormal><o:p> </o:p></p></blockquote><p \
class=MsoNormal><o:p> </o:p></p></blockquote><p \
class=MsoNormal><o:p> </o:p></p></blockquote></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic