[RFC][Icedtea-Web]: Ensure all ports entered are numeric values
Andrew Su
asu at redhat.com
Thu Mar 17 10:44:03 PDT 2011
ping.
----- Original Message -----
> ----- Original Message -----
> > From: "Omair Majid" <omajid at redhat.com>
> > To: "Andrew Su" <asu at redhat.com>
> > Cc: "Deepak Bhole" <dbhole at redhat.com>, "IcedTea"
> > <distro-pkg-dev at openjdk.java.net>
> > Sent: Monday, February 28, 2011 6:27:43 PM
> > Subject: Re: [RFC][Icedtea-Web]: Ensure all ports entered are
> > numeric values
> > On 02/28/2011 05:52 PM, Andrew Su wrote:
> > > Here is the updated version. I did not see the need to use
> > > InputVerifier for the advanced options in the network setting tab.
> > > Reason is because there was little to verify there which could be
> > > done at time of clicking "Ok". If I were to add it in there as
> > > well,
> > > there would be need to either make a subclass of InputVerifier to
> > > reduce redundant creation of anonymous classes (which was what you
> > > wanted to avoid[creating new class]).
> > >
> > > Comments question concerns about this patch?
> > >
> >
> > It would be really cool if we could indicate the invalid JTextFields
> > (or
> > other control) to the users.
>
> I had that implemented when testing it with InputVerifier, but didn't
> see that there be a need for it. It would seem a bit in consistant
> with the rest of the gui. Maybe add it as an enhancement later. :)
>
> >
> > A few other comments in-line.
>
> Most of those are residue from working in the same file for all the
> changes.
>
> >
> > >
> > > diff -r 4860276e9bf2
> > > netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> > > ---
> > > a/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> > > Sun Feb 27 21:48:17 2011 -0500
> > > +++
> > > b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> > > Mon Feb 28 17:47:41 2011 -0500
> > > @@ -27,6 +27,7 @@
> > > import java.awt.event.ActionListener;
> > > import java.awt.event.ItemEvent;
> > > import java.awt.event.ItemListener;
> > > +import java.text.ParseException;
> > > import java.util.ArrayList;
> > > import java.util.List;
> > >
> > > @@ -35,11 +36,14 @@
> > > import javax.swing.JCheckBox;
> > > import javax.swing.JComponent;
> > > import javax.swing.JDialog;
> > > +import javax.swing.JFormattedTextField;
> > > import javax.swing.JLabel;
> > > +import javax.swing.JOptionPane;
> > > import javax.swing.JPanel;
> > > import javax.swing.JScrollPane;
> > > import javax.swing.JTextArea;
> > > import javax.swing.JTextField;
> > > +import javax.swing.text.MaskFormatter;
> > >
> >
> > Some of these imports look unused - I dont see any MaskFormatter or
> > JFormattedTextField in this file. Please remove them.
> >
> > > import net.sourceforge.jnlp.config.DeploymentConfiguration;
> > > import net.sourceforge.jnlp.runtime.Translator;
> > > @@ -114,28 +118,28 @@
> > > // This addresses the HTTP proxy settings.
> > > JLabel http = new JLabel(Translator.R("APSLabelHTTP") +
> > > ":");
> > > final JTextField httpAddressField = new
> > > JTextField(fields[0]);
> > > - final JTextField httpPortField = new JTextField(fields[1]);
> > > + JTextField httpPortField = new JTextField(fields[1]);
> > > httpAddressField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 0));
> > > httpPortField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 1));
> > >
> > > // This addresses the HTTPS proxy settings.
> > > JLabel secure = new
> > > JLabel(Translator.R("APSLabelSecure")
> > > + ":");
> > > final JTextField secureAddressField = new
> > > JTextField(fields[2]);
> > > - final JTextField securePortField = new JTextField(fields[3]);
> > > + JTextField securePortField = new JTextField(fields[3]);
> > > secureAddressField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 2));
> > > securePortField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 3));
> > >
> > > // This addresses the FTP proxy settings.
> > > JLabel ftp = new JLabel(Translator.R("APSLabelFTP") +
> > > ":");
> > > final JTextField ftpAddressField = new
> > > JTextField(fields[4]);
> > > - final JTextField ftpPortField = new JTextField(fields[5]);
> > > + JTextField ftpPortField = new JTextField(fields[5]);
> > > ftpAddressField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 4));
> > > ftpPortField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 5));
> > >
> > > // This addresses the Socks proxy settings.
> > > JLabel socks = new JLabel(Translator.R("APSLabelSocks")
> > > +
> > > ":");
> > > final JTextField socksAddressField = new
> > > JTextField(fields[6]);
> > > - final JTextField socksPortField = new JTextField(fields[7]);
> > > + JTextField socksPortField = new JTextField(fields[7]);
> > > socksAddressField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 6));
> > > socksPortField.getDocument().addDocumentListener(new
> > > DocumentAdapter(fields, 7));
> > >
> >
> > Is the only change here making some final variables non-final? Is
> > there
> > a need for this?
> >
> > > @@ -243,6 +247,17 @@
> > > okButton.addActionListener(new ActionListener() {
> > > @Override
> > > public void actionPerformed(ActionEvent e) {
> > > +
> > > + try {
> > > + if (fields[1] != null) Integer.parseInt(fields[1]);
> > > + if (fields[3] != null) Integer.parseInt(fields[3]);
> > > + if (fields[5] != null) Integer.parseInt(fields[5]);
> > > + if (fields[7] != null) Integer.parseInt(fields[7]);
> > > + } catch (NumberFormatException nfe) {
> > > + JOptionPane.showMessageDialog(null,
> > > Translator.R("CPInvalidPort2"));
> > > + return;
> > > + }
> > > +
> >
> > I have a few concerns here. First, the magic constants: is there no
> > other way to validate the values? Second, while you are checking
> > that
> > ports are valid integers, you may also want to check that they are
> > valid
> > port numbers. Port numbers are 16 bit integers, while Java Integers
> > are
> > 32 bit integers - so it is possible to have values which is okay'ed
> > by
> > this code but fails later on.
>
> Not really, since the fields array is updated as the user inputs.
> (Unless I add the input verifier code to this too).
> Added a check to ensure that no value will be above 2^16-1 or below 0.
>
> >
> > > for (int i = 0; i< fields.length; i++)
> > > config.setProperty(properties[i],
> > > fields[i]);
> > >
> > > diff -r 4860276e9bf2
> > > netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > > ---
> > > a/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > > Sun Feb 27 21:48:17 2011 -0500
> > > +++
> > > b/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > > Mon Feb 28 17:47:41 2011 -0500
> > > @@ -33,9 +33,12 @@
> > >
> > > import javax.swing.Box;
> > > import javax.swing.ButtonGroup;
> > > +import javax.swing.InputVerifier;
> > > import javax.swing.JButton;
> > > import javax.swing.JCheckBox;
> > > +import javax.swing.JComponent;
> > > import javax.swing.JLabel;
> > > +import javax.swing.JOptionPane;
> > > import javax.swing.JPanel;
> > > import javax.swing.JRadioButton;
> > > import javax.swing.JTextField;
> > > @@ -114,6 +117,25 @@
> > > addressField.getDocument().addDocumentListener(new
> > > DocumentAdapter(config, properties[1]));
> > >
> > > final JTextField portField = new
> > > JTextField(config.getProperty(properties[2]), 3);
> > > + portField.setInputVerifier(new InputVerifier() {
> > > +
> > > + @Override
> > > + public boolean verify(JComponent input) {
> > > + JTextField jtf = (JTextField) input;
> > > +
> > > + try {
> > > + String str = jtf.getText();
> > > + if (str != null&& str.trim().length()> 0) Integer.parseInt(str);
> > > + return true;
> > > + } catch (NumberFormatException nfe) {
> > > + JOptionPane.showMessageDialog(null,
> > > Translator.R("CPInvalidPort1"));
> > > + config.setProperty(properties[2], "");
> >
> > Is setting the config property to "" intentional?
>
> It was suppose to be set to null, mistake.
>
> Cheers,
> Andrew
More information about the distro-pkg-dev
mailing list