[RFC][Icedtea-Web]: Ensure all ports entered are numeric values
Andrew Su
asu at redhat.com
Mon Feb 28 16:00:13 PST 2011
----- 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20110228_patch1_validate_port_text_fields_v3.patch
Type: text/x-patch
Size: 4155 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110228/f3c2e349/20110228_patch1_validate_port_text_fields_v3.patch
More information about the distro-pkg-dev
mailing list