[RFC][icedtea-web] Launch errors are not being printed to terminal.

[Omair Majid]

On 03/13/2012 05:44 PM, Danesh Dadachanji wrote:
> On 13/03/12 05:21 PM, Omair Majid wrote:
>> On 03/13/2012 03:57 PM, Danesh Dadachanji wrote:
>>> >  On 12/03/12 08:55 PM, Omair Majid wrote:
>>>> >>  On 03/12/2012 10:59 AM, Danesh Dadachanji wrote:
>>>>> >>>  I thought so too until I checked where printMessage() was
>>>>> called from.
>>>>> >>>  See launchWarning in GuiLaunchHandler[1], it explicitly calls
>>>>> >>>  DefaultLaunchHandler.printMessage(). I didn't look into why it
>>>>> was doing
>>>>> >>>  that so if you think that's a bug, I can look into it further.
>>>>> >>>
>>>> >>
>>>> >>  Assuming that we want to print the stack trace only in -verbose
>>>> mode,
>>>> >>  how does the attached patch look? (I did get a little side
>>>> tracked and
>>>> >>  fixed problems in printing the exceptions too).
>>>> >>
>>> >
>>> >  This is way better than my patch! One question, why did you choose to
>>> >  print the entire exception as opposed to just the cause? I realize
>>> >  icedtea-web does it that way and it may even be better but I found
>>> that
>>> >  when I did that to test, the cause was repeated.
>>> >
>> I am not entirely clear on this. Do you mean the stack trace was
>> repeated? As in it had duplicate "Caused by" lines?
>>
>>>> >>  I can rework this if others still think printing the entire
>>>> stack trace
>>>> >>  in -headless mode is a good idea.
>>> >
>>> >  No I think you have the right idea. I completely forgot about the
>>> "Show
>>> >  Details" button which can be seen as somewhat similar to supplying
>>> >  -verbose. I'm fine with the one-liner in output.
>>> >
>>> >
>>> >  Another thing, did you intentionally leave out the part that
>>> prints the
>>> >  exception to stderr with -verbose in GuiLaunchHandler?
>> Doh. Sorry; it was a mistake.
>>
>>> >  If not, can you
>>> >  include it in this patch?
>> Done. How does the attached patch look?
>>
> 
> Great stuff, one more nitpick below and then it's fine with me.
> 
>> diff --git a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>> b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>> --- a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
>> +++ b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java
> 
> [snip]
> 
>> @@ -71,6 +77,9 @@
>>                   BasicExceptionDialog.show(exception);
>>               }
>>           });
>> +        if (JNLPRuntime.isDebug()) {
>> +            exception.printStackTrace(outputStream);
> 
> Could you change that to printMessage(exception); instead? I think
> consistency here between what is printed to terminal between GUI and
> headless runs is better. We might base future tests off misinterpretations!

Sorry, I was a little confused (the original patch had an 'if
JNLPRuntime.isDebug()). I prefer printMessage here too because of the
consistency. I should probably add unit tests for the GUI too, except
it's a little coupled with a bunch of other things.

>> +        }
>>       }
>>
> 
> Also, you forgot to include AbstractLaunchHandler. =)
> 

Whoops. The attached patch should address both the problems.

Thanks for the multiple reviews!

Cheers,
Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: print-messages-04.patch
Type: text/x-patch
Size: 21510 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120313/f31df5f1/print-messages-04.patch