[RFC][icedtea-web]: Adding control panel to icedtea-web
Omair Majid
omajid at redhat.com
Wed Dec 1 10:17:33 PST 2010
Hi Andrew,
On 12/01/2010 10:48 AM, Andrew Su wrote:
> Here is another update to the patch with a little tweeking.
> Removed KeyListener and replaced it with KeyAdapter to help shorten the code.
> Added a few more comments.
> Moved the class from inside AdvancedProxySettingsPane.java into its own file since it is being reused elsewhere.
> Replaced MouseListener with MouseAdapter.
>
> Questions? Comments? Concerns?
>
Thanks for keeping up with all the suggestions on this patch. I have a
few comments below. These issues should be addressed, but please dont
hold up adding the control panel to icedtea-web because of just this.
I dont like how magic constants are sprinkled throughout the code :( Is
there a reason that some places create a border of size 1 while other
places create a border of size 5 (or is it 10?). I also dont like how
there are things like Dimension(500,300) used in places. Are there any
issues with just doing a pack()? IMHO, we should not make assumptions
about screen sizes (or font sizes).
> @Andrew Hughes: distcheck succeeded.
> @Omair: Thanks for suggesting to use KeyAdapter and MouseAdapter.
>
You're welcome!
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java Wed Dec 01 10:34:47 2010 -0500
> + public static void showAdvancedProxySettingsDialog(DeploymentConfiguration config) throws Exception {
> + setSystemLookAndFeel();
> +
> + AdvancedProxySettingsDialog nsd = new AdvancedProxySettingsDialog(config);
> + nsd.setResizable(false);
> + nsd.centerDialog();
> + nsd.setVisible(true);
> + nsd.dispose();
> + }
> +
I think you are violating the Swing threading rules here. Please create
JDialogs (and its subclasses) in the swing event dispatch thread only.
> + public static void main(String[] args) throws Exception {
> + final DeploymentConfiguration config = new DeploymentConfiguration();
> + config.load();
> + showAdvancedProxySettingsDialog(config);
> + }
> +}
Is this meant to be executable?
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java Wed Dec 01 10:34:47 2010 -0500
> + final JTextArea exceptionListArea = new JTextArea();
> + exceptionListArea.setLineWrap(true);
> + exceptionListArea.setText(fields[9]);
> + exceptionListArea.addKeyListener(new KeyListener() {
> + @Override
> + public void keyPressed(KeyEvent e) {}
> +
> + @Override
> + public void keyReleased(KeyEvent e) {
> + fields[9] = exceptionListArea.getText();
> + }
> +
> + @Override
> + public void keyTyped(KeyEvent e) {}
> + });
Missed opportunity for KeyAdapter ;)
> +
> + public static void main(String[] args) throws ConfigurationException {
> + final DeploymentConfiguration config = new DeploymentConfiguration();
> + config.load();
> + System.out.println("Adv Proxy Settings Test Dialog");
> + JDialog f = new JDialog();
> + f.setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE);
> + f.setPreferredSize(new Dimension(300, 300));
> + AdvancedProxySettingsPane apsp = new AdvancedProxySettingsPane(f, config);
> + f.add(apsp);
> + f.pack();
> + f.setVisible(true);
> + }
> +}
Again, is this supposed to be a standalone executable? Also, swing event
thread violations again.
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java Wed Dec 01 10:34:47 2010 -0500
> + at SuppressWarnings({ "unused", "serial" })
Why suppress unused warnings for the entire class?
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/MiddleClickListener.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/MiddleClickListener.java Wed Dec 01 10:34:47 2010 -0500
> +/**
> + * When middle click pastes to the checkboxes it doesn't register it... This is
> + * to fix that problem. Not needed in Windows. Not generic enough to be reused
> + * anywhere else, so placing it in here should be fine.
> + *
> + * @author Andrew Su<asu at redhat.com, andrew.su at utoronto.ca>
> + *
> + */
> +class MiddleClickListener extends MouseAdapter {
I dont understand what this does. Middle-click pasting to checkboxes?
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java Wed Dec 01 10:34:47 2010 -0500
> @@ -0,0 +1,298 @@
> + "deployment.proxy.bypass.local",
> + "deployment.proxy.auto.config.url", };
> + private String[] fields = new String[properties.length]; //TODO: Remove this, not used in dialog anymore.
> +
Please dont leave TODOs like this unless they refer to future work.
> + JLabel networkDesc = new JLabel("<html>" + Translator.R("CPNetworkSettingsDescription") +"<hr /></html>");
> +
> + JLabel description = new JLabel("<html>" + Translator.R("NSDescription-1") +"</html>");
> + JLabel description0 = new JLabel("<html>" + Translator.R("NSDescription0") +"</html>");
> + JLabel description1 = new JLabel("<html>" + Translator.R("NSDescription1") +"</html>");
> + JLabel description2 = new JLabel("<html>" + Translator.R("NSDescription2") +"</html>");
> + JLabel description3 = new JLabel("<html>" + Translator.R("NSDescription3") +"</html>");
> +
> + this.description = new JPanel(new CardLayout());
> + this.description.add(description, "-1");
> + this.description.add(description0, "0");
> + this.description.add(description1, "1");
> + this.description.add(description2, "2");
> + this.description.add(description3, "3");
> +
If I understand this right, it changes the text displayed in one area
when a selection changes. Are you sure this is the best way to do this?
Maybe something like a tooltip to explain an option might be better?
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/SecuritySettingsPanel.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/SecuritySettingsPanel.java Wed Dec 01 10:34:47 2010 -0500
> + // Only display the ones with properties that are valid or existent.
> + for (int i = 0; i< properties.length; i++) {
> + try {
> + String s = config.getProperty(properties[i]);
> + securityGeneralOptions[i].setSelected(Boolean.parseBoolean(s));
> + securityGeneralOptions[i].setActionCommand(properties[i]);
> + securityGeneralOptions[i].addActionListener(this);
> + c.gridy = i + 1;
> + topPanel.add(securityGeneralOptions[i], c);
> + } catch (Exception e) {
> + securityGeneralOptions[i] = null;
> + }
> + }
> +
Nice! I like how implemented extra things and added logic to make it
automagically work as soon as the right configuration options are added.
> diff -r 2faf3ab9f3c6 netx/net/sourceforge/jnlp/controlpanel/TemporaryInternetFilesPanel.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/TemporaryInternetFilesPanel.java Wed Dec 01 10:34:47 2010 -0500
> + public static void main(String[] args) throws ConfigurationException {
> + final DeploymentConfiguration config = new DeploymentConfiguration();
> + config.load();
> + JDialog f = new JDialog();
> + f.setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE);
> + f.setPreferredSize(new Dimension(500, 300));
> + TemporaryInternetFilesPanel apsp = new TemporaryInternetFilesPanel(config);
> + f.add(apsp);
> + f.pack();
> + f.setVisible(true);
> + }
> +
Swing threading violation again.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list