[RFC][icedtea-web]: Adding control panel to icedtea-web
Andrew Su
asu at redhat.com
Wed Dec 1 11:35:29 PST 2010
----- "Omair Majid" <omajid at redhat.com> wrote:
> From: "Omair Majid" <omajid at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: distro-pkg-dev at openjdk.java.net
> Sent: Wednesday, December 1, 2010 1:17:33 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [RFC][icedtea-web]: Adding control panel to icedtea-web
>
> 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).
I don't see where a border of 1 is being used. I do know that I do use three different border values.
This is to offset the a little misalignment with adding a border to a panel relative to the list on the left.
I have changed it to use 5 as consistancy with all other panels.
Without setting the dimensions, the panel that has the widest width will be used as the width. Because of this there will be no word wrap, which takes advantage of the <html> tag to do word wrapping in labels. If anything it could be resizeable so that in the case it does get covered the user can always re-adjust.
>
> > @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.
Placed it into SwingUtilities.invokeLater();
>
> > + 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 ;)
Nice catch!
>
> > +
> > + 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.
The main methods were used to test each component separately.
But I've now removed all the main methods except for ControlPanel.java
>
>
> > 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?
Removed.
>
> > 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?
This is for when pasting with middle click. So let's say I had a proxy ip that I want to copy and paste, middle clicking in gnome will paste it, but the keypress event will not be triggered. I'm a little worried about now what if they changed that to say some other button... right click?
>
> > 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.
Well it was a TODO. Finished that off now too.
>
> > + 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?
I can not say for certain this way is the best way to do it, however it makes it fairly convenient to view the items instead of just hovering.
> Maybe something like a tooltip to explain an option might be better?
I was considering adding tooltips for everything later, possibly future enhancements.
>
> > 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.
Thanks! :)
>
> > 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.
All main methods other than the one in ControlPanel.java is now removed as I stated above.
Once again another patch update, with the above changes.
Thanks for the review!
Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20101201_controlpanel_v4.patch
Type: text/x-patch
Size: 87819 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20101201/9cede2c3/20101201_controlpanel_v4.patch
More information about the distro-pkg-dev
mailing list