[rfc][icedtea-web] dialogue for set jre dir
Adam Domurad
adomurad at redhat.com
Thu Mar 21 08:27:29 PDT 2013
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?)
- I would like a message here stating that misconfiguration of the JRE
will require manual editing of the properties file.
- I liked the colourful checking, however note that as it stands we
currently only support *IcedTea*, 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.
Inline comments:
> diff -r 125c427b7a42 netx/net/sourceforge/jnlp/config/Defaults.java
> --- a/netx/net/sourceforge/jnlp/config/Defaults.java Thu Feb 14
> 15:48:21 2013 -0500
> +++ b/netx/net/sourceforge/jnlp/config/Defaults.java Fri Feb 22
> 09:45:06 2013 +0100
> @@ -384,6 +384,12 @@
> DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS,
> null,
> null
> + },
> + //JVM executable for itw
> + {
> + DeploymentConfiguration.KEY_JRE_DIR,
> + null,
> + null
> }
> };
>
> diff -r 125c427b7a42
> netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> Thu Feb 14 15:48:21 2013 -0500
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> Fri Feb 22 09:45:06 2013 +0100
> @@ -162,6 +162,7 @@
> * JVM arguments for plugin
> */
> public static final String KEY_PLUGIN_JVM_ARGUMENTS=
> "deployment.plugin.jvm.arguments";
> + public static final String KEY_JRE_DIR= "deployment.jre.dir";
>
> public enum ConfigType {
> System, User
> @@ -174,6 +175,10 @@
> private File systemPropertiesFile = null;
> /** The user's deployment.config file */
> 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'.
>
> /** the current deployment properties */
> private Map<String, Setting<String>> currentConfiguration;
> @@ -206,8 +211,7 @@
> */
> public void load(boolean fixIssues) throws ConfigurationException {
> // make sure no state leaks if security check fails later on
> - File userFile = new File(System.getProperty("user.home") +
> File.separator + DEPLOYMENT_DIR
> - + File.separator + DEPLOYMENT_PROPERTIES);
> + File userFile = new File(USER_FILE.getAbsolutePath());
>
> SecurityManager sm = System.getSecurityManager();
> if (sm != null) {
> @@ -400,8 +404,25 @@
> return etcFile;
> }
>
> - File jreFile = new File(System.getProperty("java.home") +
> File.separator + "lib"
> - + File.separator + DEPLOYMENT_CONFIG);
> + String jrePath = null;
> + try {
> + Map<String, Setting<String>> tmpProperties =
> parsePropertiesFile(USER_FILE);
> + Setting<String> jreSetting = tmpProperties.get(KEY_JRE_DIR);
> + if (jreSetting != null) {
> + jrePath = jreSetting.getValue();
> + }
> + } catch (Exception ex) {
> + ex.printStackTrace();
> + }
> +
> + File jreFile;
> + if (jrePath != null) {
> + jreFile = new File(jrePath + File.separator + "lib"
> + + File.separator + DEPLOYMENT_CONFIG);
> + } else {
> + jreFile = new File(System.getProperty("java.home") +
> File.separator + "lib"
> + + File.separator + DEPLOYMENT_CONFIG);
> + }
> if (jreFile.isFile()) {
> return jreFile;
> }
> diff -r 125c427b7a42 netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java Thu Feb
> 14 15:48:21 2013 -0500
> +++ b/netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java Fri Feb
> 22 09:45:06 2013 +0100
> @@ -40,8 +40,18 @@
> import java.awt.Dimension;
> import java.awt.GridBagConstraints;
> import java.awt.GridBagLayout;
> +import java.awt.Insets;
> +import java.awt.event.ActionEvent;
> +import java.awt.event.ActionListener;
> +import java.io.BufferedReader;
> +import java.io.File;
> +import java.io.IOException;
> +import java.io.InputStream;
> +import java.io.InputStreamReader;
>
> import javax.swing.Box;
> +import javax.swing.JButton;
> +import javax.swing.JFileChooser;
> import javax.swing.JLabel;
> import javax.swing.JTextField;
>
> @@ -50,31 +60,99 @@
>
> @SuppressWarnings("serial")
> public class JVMPanel extends NamedBorderPanel {
> - private DeploymentConfiguration config;
> +
> + private DeploymentConfiguration config;
> + private File lastPath = new File("/usr/lib/jvm/java/jre/");
> + private final JVMPanel self;
>
> JVMPanel(DeploymentConfiguration config) {
> super(Translator.R("CPHeadJVMSettings"), new GridBagLayout());
> this.config = config;
> addComponents();
> + self=this;
> }
>
> private void addComponents() {
> - JLabel description = new JLabel("<html>" +
> Translator.R("CPJVMPluginArguments") + "<hr /></html>");
> - JTextField testFieldArguments = new JTextField(25);
> + final JLabel description = new JLabel("<html>" +
> Translator.R("CPJVMPluginArguments") + "<hr /></html>");
> + final JTextField testFieldArguments = new JTextField(25);
>
> testFieldArguments.getDocument().addDocumentListener(new
> DocumentAdapter(config,
> DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS));
> testFieldArguments.setText(config.getProperty(DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS));
> +
> +
> + final JLabel descriptionExec = new JLabel("<html>" +
> Translator.R("CPJVMPluginExec") + "<hr /></html>");
> + final JTextField testFieldArgumentsExec = new JTextField(25);
> +
> + testFieldArgumentsExec.getDocument().addDocumentListener(new
> DocumentAdapter(config, DeploymentConfiguration.KEY_JRE_DIR));
> +
> testFieldArgumentsExec.setText(config.getProperty(DeploymentConfiguration.KEY_JRE_DIR));
> +
> + final JButton selectJvm = new JButton(
> Translator.R("CPJVMPluginSelectExec"));
> + final JLabel validationResult = new
> JLabel(resetValidationResult(testFieldArgumentsExec.getText(),"","CPJVMnone"));
> + selectJvm.addActionListener(new ActionListener() {
> +
> + @Override
> + public void actionPerformed(ActionEvent e) {
> + JFileChooser jfch;
> + if (lastPath != null && lastPath.exists()){
> + jfch = new JFileChooser(lastPath);
> + } else {
> + jfch = new JFileChooser();
> + }
> + jfch.setFileSelectionMode(JFileChooser.DIRECTORIES_ONLY);
> + int i = jfch.showOpenDialog(self);
> + if (i == JFileChooser.APPROVE_OPTION &&
> jfch.getSelectedFile()!=null){
> + lastPath=jfch.getSelectedFile().getParentFile();
> + String nws = jfch.getSelectedFile().getAbsolutePath();
> + String olds = testFieldArgumentsExec.getText();
> + if (!nws.equals(olds)){
> +
> validationResult.setText(resetValidationResult(testFieldArgumentsExec.getText(),"","CPJVMnone"));
> + }
> + testFieldArgumentsExec.setText(nws);
> + }
> +
> + }
> +
> + });
> + final JButton validateJvm = new JButton(
> Translator.R("CPJVMPluginExecValidation") );
> + validateJvm.addActionListener(new ActionListener() {
> +
> + @Override
> + public void actionPerformed(ActionEvent e) {
> + String s = validateJvm(testFieldArgumentsExec.getText());
> +
> validationResult.setText(resetValidationResult(testFieldArgumentsExec.getText(),s,"CPJVMvalidated"));
> +
> + }
> +
> + });
>
> // Filler to pack the bottom of the panel.
> GridBagConstraints c = new GridBagConstraints();
> c.fill = GridBagConstraints.BOTH;
> c.weightx = 1;
> + c.gridwidth = 2;
> c.gridx = 0;
> c.gridy = 0;
> + c.insets = new Insets(2, 2, 4, 4);
>
> this.add(description, c);
> c.gridy++;
> this.add(testFieldArguments, c);
> + c.gridy++;
> + this.add(descriptionExec, c);
> + c.gridy++;
> + this.add(testFieldArgumentsExec, c);
> + c.gridy++;
> + GridBagConstraints cb1 = (GridBagConstraints) c.clone();
> + cb1.fill = GridBagConstraints.NONE;
> + cb1.gridwidth = 1;
> + this.add(selectJvm, cb1);
> + GridBagConstraints cb2 = (GridBagConstraints) c.clone();
> + cb2.fill = GridBagConstraints.NONE;
> + cb2.gridx = 1;
> + cb2.gridwidth = 1;
> + this.add(validateJvm, cb2);
> + c.gridy++;
> + this.add(validationResult, c);
>
> // This is to keep it from expanding vertically if resized.
> Component filler = Box.createRigidArea(new Dimension(1, 1));
> @@ -82,4 +160,88 @@
> c.weighty++;
> this.add(filler, c);
> }
> +
> + private static String validateJvm(String cmd) {
> + if (cmd == null || cmd.trim().equals("")){
> + return "<span
> color=\"orange\">"+Translator.R("CPJVMvalueNotSet")+"</span>";
> + }
> + String procesString="";
s/procesString/processString/, although to me this seems like
'validationResult' or something.
> + 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.
> + 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.
> + 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?)
> +
> + 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)'
> +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.
> +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).
> +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...
It really is quite good, though will be better with my suggestions :-D
-Adam
More information about the distro-pkg-dev
mailing list