[RFC][IcedTea-Web]: Moved creation of swing thread.

[Andrew Su]

----- Original Message -----
> From: "Omair Majid" <omajid at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: "distro-pkg-dev" <distro-pkg-dev at openjdk.java.net>
> Sent: Tuesday, December 21, 2010 4:25:39 PM
> Subject: Re: [RFC][IcedTea-Web]: Moved creation of swing thread.
> On 12/17/2010 02:47 PM, Andrew Su wrote:
> > Hello,
> >
> > I noticed a minor issue with updating the fields when using the
> > advanced proxy dialog, fields in the original network panel was not
> > being updated when closing the dialog box. Cause: dialog ran in a
> > new
> > thread, code for updating finished before finishing dialog.
> >
> > The attached patch fixes this issue by moving the swing threading
> > out
> > to where the call happens.
> >
> > Questions? Comments? Concerns?
> > Cheers,
> >    Andrew
> >
> >
> > 20101217_moved_swing_threading.patch
> >
> >
> > diff -r 7d25178e476d ChangeLog
> > --- a/ChangeLog Fri Dec 17 14:32:17 2010 -0500
> > +++ b/ChangeLog Fri Dec 17 14:44:37 2010 -0500
> > @@ -1,3 +1,11 @@
> > +2010-12-17 Andrew Su<asu at redhat.com>
> > +
> > + *
> > netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java:
> > + (showAdvancedProxySettingsDialog): Removed creation of swing
> > thread.
> > + *
> > netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java:
> > + (addComponents): Added creation of swing thread for opening proxy
> > + dialog.
> > +
> >   2010-12-17 Andrew Su<asu at redhat.com>
> >
> >   	*
> >   	netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsPane.java
> > diff -r 7d25178e476d
> > netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> > ---
> > a/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> > Fri Dec 17 14:32:17 2010 -0500
> > +++
> > b/netx/net/sourceforge/jnlp/controlpanel/AdvancedProxySettingsDialog.java
> > Fri Dec 17 14:44:37 2010 -0500
> > @@ -120,16 +120,12 @@
> >        */
> >       public static void showAdvancedProxySettingsDialog(final
> >       DeploymentConfiguration config) throws Exception {
> >           setSystemLookAndFeel();
> 
> As I said before, please remove these redundant calls to
> UIManager.setLookAndFeel().
This will be done in a separate patch.

> 
> > - SwingUtilities.invokeLater(new Runnable() {
> > - @Override
> > - public void run() {
> > - AdvancedProxySettingsDialog psd = new
> > AdvancedProxySettingsDialog(config);
> > - psd.setResizable(false);
> > - psd.centerDialog();
> > - psd.setVisible(true);
> > - psd.dispose();
> > - }
> > - });
> > +
> > + AdvancedProxySettingsDialog psd = new
> > AdvancedProxySettingsDialog(config);
> > + psd.setResizable(false);
> > + psd.centerDialog();
> > + psd.setVisible(true);
> > + psd.dispose();
> >
> 
> This is supposed to be a modal dialog, right? In that case, you may
> want
> to have an additional argument for the parent window/dialog, and
> create
> this dialog as modal (see JDialog.setModalityType) so that it blocks
> the
> parent. This should fix the bug.
> 
> >       }
> >
> > diff -r 7d25178e476d
> > netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > ---
> > a/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > Fri Dec 17 14:32:17 2010 -0500
> > +++
> > b/netx/net/sourceforge/jnlp/controlpanel/NetworkSettingsPanel.java
> > Fri Dec 17 14:44:37 2010 -0500
> > @@ -42,6 +42,7 @@
> >   import javax.swing.JPanel;
> >   import javax.swing.JRadioButton;
> >   import javax.swing.JTextField;
> > +import javax.swing.SwingUtilities;
> >
> >   import net.sourceforge.jnlp.config.DeploymentConfiguration;
> >   import net.sourceforge.jnlp.runtime.Translator;
> > @@ -115,22 +116,27 @@
> >           JLabel portLabel = new JLabel(Translator.R("NSPort") +
> >           ":");
> >           final JTextField addressField = new
> >           JTextField(config.getProperty(properties[1]), 10);
> >           addressField.getDocument().addDocumentListener(new
> >           DocumentAdapter(config, properties[1]));
> > -
> > +
> >           final JTextField portField = new
> >           JTextField(config.getProperty(properties[2]), 3);
> >           portField.getDocument().addDocumentListener(new
> >           DocumentAdapter(config, properties[1]));
> > -
> > +
> >           // Create the button which allows setting of other types
> >           of proxy.
> >           JButton advancedProxyButton = new
> >           JButton(Translator.R("NSAdvanced") + "...");
> >           advancedProxyButton.addActionListener(new ActionListener()
> >           {
> >               @Override
> >               public void actionPerformed(ActionEvent e) {
> > - try {
> > -
> > AdvancedProxySettingsDialog.showAdvancedProxySettingsDialog(config);
> > - addressField.setText(config.getProperty(properties[1]));
> > - portField.setText(config.getProperty(properties[2]));
> > - } catch (Exception e1) {
> > - e1.printStackTrace();
> > - }
> > + SwingUtilities.invokeLater(new Runnable() {
> > + @Override
> > + public void run() {
> > + try {
> > +
> > AdvancedProxySettingsDialog.showAdvancedProxySettingsDialog(config);
> > + addressField.setText(config.getProperty(properties[1]));
> > + portField.setText(config.getProperty(properties[2]));
> > + } catch (Exception e1) {
> > + e1.printStackTrace();
> > + }
> > + }
> > + });
> >               }
> >           });
> >
> 
> AFAIK, listeners are run in the swing event dispatch thread. There
> should be no need to use SwingUtilities.invokeLater() here.
There was some miscommunication about this before, this updated patch will make it work as intended and not need to create extra threads.

Cheers,
  Andrew

> 
> Cheers,
> Omair
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20101223_remove_need_for_creating_swing_thread.patch
Type: text/x-patch
Size: 2504 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20101223/5658c410/20101223_remove_need_for_creating_swing_thread.patch