[rfc][icedtea-web] dialogue for set jre dir
Jiri Vanek
jvanek at redhat.com
Fri Mar 29 08:49:16 PDT 2013
On 03/21/2013 04:27 PM, Adam Domurad wrote:
> On 03/20/2013 06:54 AM, Jiri Vanek wrote:
>>
>> ping?
>>
>> -------- Original Message --------
>> Subject: [rfc][icedtea-web] dialogue for set jre dir
>> Date: Fri, 22 Feb 2013 15:01:17 +0100
>> From: Jiri Vanek <jvanek at redhat.com>
>> To: IcedTea Distro List <distro-pkg-dev at openjdk.java.net>
>>
>> This is only java part of "make-jredir-configurable after install effort"
>> To jvm settings pane it adds text-field to allow to write path to jre, whih is then saved to
>> properties. There is also button which fill launch jfilechooser (with correct default location) to
>> allow to choose the jre directory
>> And one funny button to validate his effort (with funny colourful messages;). As if the user will
>> add wrong location, he will not even start itw-settings (later).
>>
>>
>> J.
>>
>>
>>
>
> It looked quite well done. I liked the visual validation a lot. General comments:
>
> - We should validate as soon as the user picks a path, and we should not let the user accept a
> configuration that couldn't possibly work. (itweb-settings will not be able to save them from a
> broken jre dir, correct?)
Ok. Although I don like it, added (added checkbox to disable type-time validation)
> - I would like a message here stating that misconfiguration of the JRE will require manual editing
> of the properties file.
God idea. added. Can also cancel closing of dialogue with invalid value.
>
> - I liked the colourful checking, however note that as it stands we currently only support
> *IcedTea*,
Not true. We support OpenJdk.
>not just some hypothetical OpenJDK without our applet-related patches. We really should
> fail on anything else until the plugin/javaws is more flexible. It's a bit sad, because javaws could
> work with at the very least the Oracle JRE if NetxPanel were with the rest of the sun.applet.* package.
> - Although I see messages for other JRE's other than OpenJDK, it failed to detect my Oracle JRE.
> However, see above comment on why we shouldn't accept this anyway.
Yah :((
I have fixed detection of oracle jdk.
>
>
>> private File userPropertiesFile = null;
>> +
>> + /*default user file*/
>> + public static final File USER_FILE = new File(System.getProperty("user.home") +
>> File.separator + DEPLOYMENT_DIR
>> + + File.separator + DEPLOYMENT_PROPERTIES);
>
> USER_DEPLOYMENT_PROPERTIES is better ? Or something like that. USER_FILE doesn't really 'speak to me'.
fixed.
>
...
>> + String procesString="";
>
> s/procesString/processString/, although to me this seems like 'validationResult' or something.
fixed
>
>> + File jreDirFile = new File (cmd);
>> + if (jreDirFile.isDirectory()) {
>> + procesString+="<span color=\"green\">"+Translator.R("CPJVMisDir")+"</span><br />";
>> + }else{
>> + procesString+="<span color=\"red\">"+Translator.R("CPJVMnotDir")+"</span><br />";
>> + }
>> + File javaFile = new File (cmd+File.separator+"bin"+File.separator+"java");
>> + if (javaFile.isFile()) {
>> + procesString+="<span color=\"green\">"+Translator.R("CPJVMjava")+"</span><br />";
>> + }else{
>> + procesString+="<span color=\"red\">"+Translator.R("CPJVMnoJava")+"</span><br />";
>> + }
>> + File rtFile = new File (cmd+File.separator+"lib"+File.separator+"rt.jar");
>> + if (javaFile.isFile()) {
>> + procesString+="<span color=\"green\">"+Translator.R("CPJVMrtJar")+"</span><br />";
>> + }else{
>> + procesString+="<span color=\"red\">"+Translator.R("CPJVMnoRtJar")+"</span><br />";
>> + }
>> + ProcessBuilder sb = new ProcessBuilder(javaFile.getAbsolutePath(), "-version");
>> + Process p = null;
>> + String s1 ="";
>> + String s2 = "";
>
> These strings could use more descriptive names. I'm in favour of 'stdOut' and 'stdErr' or similar.
ok :)
>
>> + Integer r = null;
>> + try {
>> + p = sb.start();
>> + p.waitFor();
>> + s1 = inputToHtmlString(p.getErrorStream());
>> + s2 = inputToHtmlString(p.getInputStream());
>> + r = p.exitValue();
>> + System.err.println(s1);
>> + System.out.println(s2);
>> + s1=s1.toLowerCase();
>> + s2=s2.toLowerCase();
>> + } catch (Exception ex) {;
>> + ex.printStackTrace();
>> +
>> + }
>> + if (r == null){
>> + return procesString+"<span color=\"red\">"+Translator.R("CPJVMnotLaunched")+"</span>";
>> + }
>> + if (r.intValue()!=0){
>> + return procesString+"<span color=\"red\">"+Translator.R("CPJVMnoSuccess")+"</span>";
>> + }
>> + if (s1.contains("openjdk") || s2.contains("openjdk")){
>> + return procesString+"<span
>> color=\"#00FF00\">"+Translator.R("CPJVMopenJdkFound")+"</span>" ;
>> + }
>
> [nit] I found this green hard on the eyes.
Lowered to 00EE00
>
>> + if (s1.contains("oracle") || s2.contains("oracle")){
>> + return procesString+"<span color=\"green\">"+Translator.R("CPJVMoracleFound")+"</span>" ;
>> + }
>> + if (s1.contains("ibm") || s2.contains("ibm")){
>> + return procesString+"<span color=\"green\">"+Translator.R("CPJVMibmFound")+"</span>";
>> + }
>> + if (s1.contains("gij") || s2.contains("gij")){
>> + return procesString+"<span color=\"orange\">"+Translator.R("CPJVMgijFound")+"</span>";
>> + }
>> + return procesString+"<span color=\"orange\">"+Translator.R("CPJVMstrangeProcess")+"</span>";
>> +
>> + }
>> +
>> + private static String inputToHtmlString(InputStream stream) throws IOException {
>> + InputStreamReader is = new InputStreamReader(stream);
>> + StringBuilder sb = new StringBuilder();
>> + BufferedReader br = new BufferedReader(is);
>> + while (true) {
>> + String read = br.readLine();
>> + if (read == null) {
>> + break;
>> + }
>> + sb.append(read);
>> +
>> + }
>> +
>> + return sb.toString();
>> + }
>
> I don't see anything related to HTML here. In fact, I think this is a good candidate for placement
> in the StreamUtils class. (readStreamAsString or similar, I think?)
ok. Done.
>
>> +
>> + private String resetValidationResult(final String value, String result, String headerKey) {
>> + return "<html>" + Translator.R(headerKey) + ": <br />"+value+" <br />"+result+"<hr
>> /></html>";
>> + }
>> }
>
>
> The Swing code looked otherwise fine to me, although I tend to not like to stare at Swing code too
> long :-).
>
>> diff -r 125c427b7a42 netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Thu Feb 14 15:48:21 2013 -0500
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Fri Feb 22 09:45:06 2013 +0100
>> @@ -301,6 +301,25 @@
>> CPDebuggingDescription=Enable options here to help with debugging
>> CPDesktopIntegrationDescription=Set whether or not to allow creation of desktop shortcut.
>> CPJVMPluginArguments=Set JVM arguments for plugin.
>> +CPJVMPluginExec=Set JVM for icedtea-web
>
> The difference between 'plugin' and 'icedtea-web' setting may not be clear, I'd like to see 'Set JVM
> for icedtea-web (plugin and javaws)'
If you are refering to CPJVMPluginExec and CPJVMPluginExecValidationthen you are right.
If you are referring to CPJVMPluginArguments, then I'm afraid javaws do not read this. But should!
Worthy of another, simple patch?
Or add CPJVMJavawsArguments ?
>
>> +CPJVMPluginExecValidation=Validate JVM for icedtea-web
>> +CPJVMPluginSelectExec=Select JVM for icedtea-web
>> +CPJVMnone=None validation result for
>
> I'm sorry, I have no clue what 'None validation result' means or why it is needed. From when I saw
> it occur, it seems nothing be outputted would suffice.
It is used whn there is no valid result (eg erro during processing or no invocation of validation)
kept.
>
>> +CPJVMvalidated=Validation result for
>> +CPJVMvalueNotSet=Value is not set. Hardcoded JVM will be used
>> +CPJVMnotLaunched=Error, process was not launched, see console outputs for more info
>
> s/outputs/output/
>
>> +CPJVMnoSuccess=Error, process have not ended successfully, see output for details, but your java
>> is not set correctly
>
> It's a nit, but I'd like these all to end with periods. Especially the messages with more than one
> sentence.
>
>> +CPJVMopenJdkFound=Excellent, OpenJDK detected
>> +CPJVMoracleFound=Great, Oracle java detected
>> +CPJVMibmFound=Good, IBM java detected
>> +CPJVMgijFound=Warning, gij detected
>
> These messages are a little too 'funny' IMHO. Anyway as stated, we should have exactly one correct
> state, 'Valid JRE, IcedTea was successfully detected'. You should really just have one message
> 'Valid JRE, {0} was successfully detected', to ease translation efforts. GIJ never got past java5,
> there's no need to do anything but fail on it (although we can say why).
Not mentioned to be funny. Little bit reworded and hints leading to OpenJDK added.
>
>> +CPJVMstrangeProcess=Your path is executable process, but was not recognized. Ensure yourself in
>> console output
>
> I favour ' Your path had an executable process, but it was not recognized. Verify the Java version
> in the console output.'
> We can also print the version right on the window, although I don't think it's strictly necessary.
>
>> +CPJVMnotDir=You must set directory
>
> I favour 'Problem: The path you chose was not a directory.'
>
>> +CPJVMisDir=Is directory
>
> I favour 'The path you chose was a directory, check.'
>
>> +CPJVMnoJava=Must contains bin/java
>
> s/contains/contain/
> I favour 'Problem: The directory you choose must contain bin/java.'
>
>> +CPJVMjava=Contains bin/java
>
> s/contains/contain/
> I favour 'The directory you chose contains bin/java, check.'
>
>> +CPJVMnoRtJar=Do not contains lib/rt.jar
>
> s/Do not contains/Does not contain/
> I favour 'Problem: The directory you chose does not contain bin/java.'
>
>> +CPJVMrtJar=Do contains lib/rt.jar
>
> s/contains/contain/
> I favour 'The directory you chose contains bin/java, check.'
>
>>
>> # Control Panel - Buttons
>> CPButAbout=About...
>
Mostly followed the new sentences. Thanx for review!
> It really is quite good, though will be better with my suggestions :-D
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: setupaAbleJVM-itwsettings_2.diff
Type: text/x-patch
Size: 22332 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130329/b25dc8a6/setupaAbleJVM-itwsettings_2.diff
More information about the distro-pkg-dev
mailing list