[rfc][icedtea-web] headless dialogues -tech preview
Andrew Azores
aazores at redhat.com
Mon Jun 15 15:36:54 UTC 2015
On 15/06/15 11:21 AM, Jiri Vanek wrote:
> On 06/15/2015 04:59 PM, Andrew Azores wrote:
>> Hi,
>>
>> On 14/06/15 02:04 PM, Jiri Vanek wrote:
>>> Hi!
>>>
>>> the dialog subsytem in itw is wrong.
>>> In last few and in a lot of future patches I'm trying to replace it.
>>> When I was doing the remeberable solution, I noted that headless
>>> uspport is completly wrong, and
>>> no worlaround may fix it.
>>> Th eonly correct reaction is to print dialog to stdou, and request
>>> user to type answer. +1 from
>>> me, her eis implementation :)
>>> It consist 9again) from little bit more work, soI will push it in
>>> parts.
>>> This patch add the headless logic, and have few advantages:
>>> - no aditional text hanling. The dialogs can prints themselves
>>> based on current ui!
>>> - no aditional input chekcing. With strong types, each can read
>>> itslef perfectly as it is already
>>> done!
>>>
>>> - it encapsualtes a lot of suplicated code - eg the if
>>> (!sahouldAskUser)...if (trustAll) ..
>>> If9trustNone).,... and more. So there is much more removed code then
>>> added :)
>>>
>>>
>>> So the patch itself is quite good, isnt it? :)
>>>
>>> Few drawbacks
>>> - stdin get broken if underlying application creates new VM.
>>> Currently I dont know how to
>>> workaround it. But it may be surviable.
>>> - it do nto work without X....
>>>
>>>
>>> yah the last sentence is pitfall. It is casued by EVERYTHING in itw
>>> expects SecurityDialogue to be
>>> existing. And new Window() is on the blacklist in headless mode.
>>> But I have some ideas how to ix it....
>>>
>>>
>>> J.
>>
>> Very cool ideas here! I think I like the approach in general and some
>> of the refactoring is very
>> nice. I do have some issues with the patch though:
>>
>>> +
>>> OutputController.getLogger().printOutLn(dialog.getText().replace("<html>",
>>> "").replace("</html>", "")); //see htmlize.. but eg a href is ok to
>>> keep
>>
>> I think the HTML-cleaning needs to be more rigorous than this. Sample
>> below, but to me this really
>> isn't sufficient and the output isn't clean looking.
>>
>
> was improved in v2 i sent few minuts ago.
>>> + OutputController.getLogger().printOutLn("Type 'exit' to terminate
>>> ITW one f below values:");
>>
>> The print message here isn't really clear... perhaps "Type one of the
>> below values, or 'exit' to
>> cancel loading the [applet|application]" ? And should this message
>> and others like it be in
>> Messages.properties?
>
> alrready moved to properties in v2. Ans sure, will be reworded.
>>
>>> + boolean keepGoing = true;
>>> + do {
>>> + try {
>>> +
>>> OutputController.getLogger().printOutLn(dialog.getText().replace("<html>",
>>> "").replace("</html>", "")); //see htmlize.. but eg a href is ok to
>>> keep
>>> + OutputController.getLogger().printOutLn("Type 'exit' to terminate
>>> ITW one f below values:");
>>> + OutputController.getLogger().printOutLn(dialog.helpToStdIn());
>>> + String s = br.readLine();
>>> + if (s==null){
>>> + break;
>>> + }
>>> + if
>>> (s.trim().toLowerCase().equals("exit")) {
>>> + JNLPRuntime.exit(0);
>>> + }
>>> + message.userResponse =
>>> dialog.readFromStdIn(s);
>>> + keepGoing = false;
>>> + //br.close(); we cant close, it closes
>>> underlyig stream
>>> + } catch (IOException eex) {
>>> + OutputController.getLogger().log(eex);
>>> + keepGoing = false;
>>> + //br.close(); we cant close, it closes
>>> underlyig stream
>>> + } catch (Exception ex) {
>>> +
>>> OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL,
>>> ex.toString());
>>> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL,
>>> ex);
>>> + }
>>> + } while (keepGoing);
>>
>> Perhaps rather than looping the whole thing and reprinting the
>> "dialog", can you just reprint the
>> prompt for input? And related, as it is now, it's not very obvious
>> when I input an invalid response
>> - I get a stack trace that makes perfect sense to me but I'd expect
>> to make zero sense to the
>> average end user:
>
> I had it like this in early stage of this patch. But becasuse whole
> exception is printed in case wrong output, it may be hard to recall
> what the I';m answering.
> Conisdering loops of
> message, help+suggested values, pass/exception
> help+suggested values, pass/exception
> message, help+suggested values, pass/better message
> help+suggested values, pass/better message
>
> I choose used "message, help+suggested values, pass/exception"
>
> My reasons are - smallest amount of work now. Straightforward and
> working.
> and - most important - headless ITW will not be run by average user :)
I really think it's worthwhile to take the "better message" approach
rather than directly presenting the user with a stacktrace. If you wish
then you can proceed how you are now and I will work on improving this
later.
>>
>>> Type 'exit' to terminate ITW one f below values:
>>> [YES, NO]
>>> NotARealResponse
>>> java.lang.IllegalArgumentException: No enum constant
>>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue.Primitive.NotARealResponse
>>>
>>> java.lang.IllegalArgumentException: No enum constant
>>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue.Primitive.NotARealResponse
>>>
>>> at java.lang.Enum.valueOf(Enum.java:238)
>>> at
>>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue$Primitive.valueOf(BasicDialogValue.java:43)
>>>
>>>
>>> at
>>> net.sourceforge.jnlp.security.dialogresults.YesNo.readValue(YesNo.java:52)
>>> at
>>> net.sourceforge.jnlp.security.dialogs.MissingPermissionsAttributePanel.readFromStdIn(MissingPermissionsAttributePanel.java:197)
>>>
>>>
>>> at
>>> net.sourceforge.jnlp.security.SecurityDialog.readFromStdIn(SecurityDialog.java:430)
>>> at
>>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.handleMessage(SecurityDialogMessageHandler.java:161)
>>>
>>>
>>> at
>>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.run(SecurityDialogMessageHandler.java:95)
>>>
>>> at java.lang.Thread.run(Thread.java:745)
>>
>> This is also printed above the reprinted dialog, so it would be easy
>> to miss.
>>
>>> + return "You can type YES/NO or komplex answe parsable by
>>> AccessWarningPaneComplexReturn.readValue.";
>>
>> This really needs to be rewritten. I don't think anything after
>> "YES/NO" is going to make sense to
>> end users - how are they supposed to know what that readValue method
>> is able to parse?
>>
>>> + //this is default SecurityDialog "toString". All extending
>>> panels are recommanded to override
>>> this.
>>
>> Maybe this would be better as a Javadoc comment?
>
> sure
>>
>>> + //But what the hhell, it is working awesom!
>>
>> And maybe this one is better removed...
>
> Rather just typo free (as it is in v2) I would like to keep similar
> comment anyway...
>>
>>> + public String getText() {
>>> + return traverse(this);
>>> + }
>>
>>> + @Override
>>> + public DialogResult getDefaultNegativeAnswer() {
>>> + return YesNoSandboxLimited.no();
>>> + }
>>
>> There are a lot of instances of weird indentation or lack of inner
>> whitespace in this patch, like
>> the override annotation here being two spaces over. Can you have your
>> IDE apply autoformatting to
>> each touched file please?
>
> I usually do :( Exactly this ">> + @Override" +-2sapces is
> slipping pretty often. When I will post this part, I will double check.
>>
>>> diff -r 0afcc93fb7fb
>>> tests/netx/unit/net/sourceforge/jnlp/security/dialogresults/NamePasswordTest.java
>>>
>>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>>> +++
>>> b/tests/netx/unit/net/sourceforge/jnlp/security/dialogresults/NamePasswordTest.java
>>> Sun Jun
>>> 14 19:52:22 2015 +0200
>>> @@ -0,0 +1,57 @@
>>> +/*
>>> + Copyright (C) 2009 Red Hat, Inc.
>>
>> Is it? :)
> I hate those headers:( So just close, but yes.. :(you are right...
>>
>>
>> Here's the sample "dialog" on stdout that I'm looking at right now:
>>
>>> $ ./bin/javaws -headless
>>> ~/Downloads/1d_fast_fourier_transform_java_jnlp.jnlp
>>> This application does not specify a Codebase in its manifest. Please
>>> verify with the applet's
>>> vendor. Continuing. See:
>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html
>>> for details.
>>> This application does not specify a Codebase in its manifest. Please
>>> verify with the applet's
>>> vendor. Continuing. See:
>>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html
>>> for details.
>>> Application title was not found in manifest. Check with application
>>> vendor
>>> Application title was not found in manifest. Check with application
>>> vendor
>>> Application <span color='red'> 1D Fast Fourier Transform </span>
>>> from <span color='red'>
>>> http://www.cs.brown.edu/exploratories/freeSoftware/repository/
>>> </span> is missing the permissions
>>> attribute. Applications without this attribute should not be
>>> trusted. Do you wish to allow this
>>> application to run?Application <span color='red'> 1D Fast Fourier
>>> Transform </span> from <span
>>> color='red'>
>>> http://www.cs.brown.edu/exploratories/freeSoftware/repository/
>>> </span> is missing the
>>> permissions attribute. Applications without this attribute should
>>> not be trusted. Do you wish to
>>> allow this application to run?
>>>
>>> <head>
>>>
>>> </head>
>>> <body>
>>> For more information you can visit:<br><a
>>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions">JAR
>>>
>>>
>>> File Manifest Attributes</a><br>and<br><a
>>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html">Preventing
>>>
>>>
>>> the repurposing of Applications</a>
>>> </body>
>>>
>>>
>>> <head>
>>>
>>> </head>
>>> <body>
>>> For more information you can visit:<br><a
>>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions">JAR
>>>
>>>
>>> File Manifest Attributes</a><br>and<br><a
>>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html">Preventing
>>>
>>>
>>> the repurposing of Applications</a>
>>> </body>
>>>
>>>
>>> YesYes
>>> NoNo
>>> <b>Remember this option?</b><b>Remember this option?</b>
>>> For appletFor applet
>>> For site
>>> <u>http://www.cs.brown.edu/exploratories/freeSoftware/repository/</u>For
>>> site
>>> <u>http://www.cs.brown.edu/exploratories/freeSoftware/repository/</u>
>>>
>>> Type 'exit' to terminate ITW one f below values:
>>> [YES, NO]
>>
>> The HTML tags don't really look very good, and most things that are
>> printed are printed twice. I
>> don't know what's causing that and how easy it is to fix or work
>> around, but I think that's a
>> blocker for this patch.
>
> This is not bug, its an feature!-)
>
> You can see how this text is composed? Also do you realize target
> audience of this patch? Also have you seen big comment " //this is
> default SecurityDialog "toString". All extending panels are
> recommanded to override " ?
>
> So this method is providing some *good* default text. (in v2 the
> removal of html tags was much improved.
Yes, the HTML tag removal does look much better now, although I'd still
personally like to see it cleaned up further. Still, this is better now.
But I do think that duplicating everything is not very nice. Sure the
target audience is not the average end user but "YesYes\nNoNo" is going
to give the more advanced user target audience the impression that this
feature wasn't really polished before release, no?
>
> The providing of texts for exactly those messages will be work for
> another changesets. MAybe for another person...
>
> Well - the headless dialogues are now completly broken, so *anything*
> is better then current state.
True, but if something's worth doing then it's worth doing correctly, right?
>>
>> Also, here's another bug for you:
>>
>>> Type 'exit' to terminate ITW one f below values:
>>> [YES, NO]
>>> netx: Initialization Error: Could not initialize applet. (Your
>>> Extended applets security is at
>
> yes I'm aware of it. I fixed it in v2. Bad typo on last second before
> posting :(
"Type exit to terminate ITW one of below values" still doesn't quite
make sense.
"Type exit to terminate ITW, or type one of the below values" would be a
fixed-up version of the same message you have now.
>
>>> 'high' and this application is missing the 'permissions' attribute
>>> in manifest. And you have
>>> refused to run it.)
>>> netx: Initialization Error: Could not initialize applet. (Your
>>> Extended applets security is at
>>> 'high' and this application is missing the 'permissions' attribute
>>> in manifest. And you have
>>> refused to run it.)
>>> net.sourceforge.jnlp.LaunchException: Fatal: Initialization Error:
>>> Could not initialize applet.
>>> For more information click "more information button".
>>> at net.sourceforge.jnlp.Launcher.createApplet(Launcher.java:764)
>>> at net.sourceforge.jnlp.Launcher.launchApplet(Launcher.java:653)
>>> at net.sourceforge.jnlp.Launcher$TgThread.run(Launcher.java:939)
>>> Caused by: net.sourceforge.jnlp.LaunchException: Your Extended
>>> applets security is at 'high' and
>>> this application is missing the 'permissions' attribute in manifest.
>>> And you have refused to run it.
>>> at
>>> net.sourceforge.jnlp.runtime.ManifestAttributesChecker.checkPermissionsAttribute(ManifestAttributesChecker.java:279)
>>>
>>>
>>> at
>>> net.sourceforge.jnlp.runtime.ManifestAttributesChecker.checkAll(ManifestAttributesChecker.java:114)
>>>
>>> at
>>> net.sourceforge.jnlp.runtime.JNLPClassLoader.initializeResources(JNLPClassLoader.java:777)
>>> at
>>> net.sourceforge.jnlp.runtime.JNLPClassLoader.<init>(JNLPClassLoader.java:285)
>>> at
>>> net.sourceforge.jnlp.runtime.JNLPClassLoader.createInstance(JNLPClassLoader.java:357)
>>> at
>>> net.sourceforge.jnlp.runtime.JNLPClassLoader.getInstance(JNLPClassLoader.java:429)
>>> at
>>> net.sourceforge.jnlp.runtime.JNLPClassLoader.getInstance(JNLPClassLoader.java:403)
>>> at net.sourceforge.jnlp.Launcher.createApplet(Launcher.java:729)
>>> ... 2 more
>>
>> I typed "Control-D"/EOF character for this. At this point it seems
>> ITW is hanging.
>>
>
> Atatched v2 again.
EOF char for input still hangs it.
>
>
> Thanx!
>
> J.
--
Thanks,
Andrew Azores
More information about the distro-pkg-dev
mailing list