[RFC][icedtea-web]: Adding control panel to icedtea-web

[Andrew Su]

----- "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: Friday, November 26, 2010 4:49:22 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [RFC][icedtea-web]: Adding control panel to icedtea-web
>
> On 11/26/2010 04:11 PM, Andrew Su wrote:
> > Hello,
> >
> > I have attached a patch to add a control panel for icedtea-web. This
> can be used to modify deployments.properties.
> >
> > Current features:
> >   - Enable/Disable caching.
> >   - Set cache location.
> >   - Set max space cache may use. (in MB)
> >   - Set compression level of jar files.
> >   - Import/View/Export/Remove certificates.
> >   - Turn tracing on/off.
> >   - Turn debugging on/off.
> >   - Set behaviour of Java Console.
> >   - Set whether to create desktop shortcut for javaws or not.
> >   - Set proxy settings.
> >   - Set how security warning behaviours.
> >
> > ChangeLog:
> >
> > 	* Makefile.am:
> > 	(CONTROLPANEL_DIR): Build directory for control panel.
> > 	(CONTROLPANEL_SRCDIR): Source directory for control panel.
> > 	(CONTROLPANEL_LAUNCHER_OBJECTS): Objects used to compile binary
> > 	control panel.
> > 	(all-local): Add stamps/controlpanel-dist.stamp
> > 	stamps/controlpanel.stamp
> $(CONTROLPANEL_DIR)/launcher/controlpanel.
> > 	(clean-local): Add clean-controlpanel.
> > 	(install-exec-local): Install the control panel jar file and
> control
> > 	panel binary.
> > 	(uninstall-local): Delete controlpanel.jar and controlpanel binary
> > 	from installed location.
> > 	(controlpanel-source-files.txt): Get the list of source files for
> > 	control panel.
> > 	(stamps/controlpanel.stamp): Compile the java files for control
> panel.
> > 	(stamps/controlpanel-dist.stamp): Depend on
> stamps/controlpanel.stamp.
> > 	Create the jar file for control panel.
> > 	($(CONTROLPANEL_DIR)/launcher/%.o): Create the launcher objects.
> > 	($(CONTROLPANEL_DIR)/launcher/controlpanel): Link the objects to
> make
> > 	the launcher.
> > 	(clean-controlpanel): Remove the compiled control panel.
> > 	(controlpanel): Calls stamps/controlpanel.stamp.
> > 	(controlpanel-dist): Calls stamps/controlpanel-dist.stamp.
> > 	* controlpanel/org/classpath/controlpanel/AboutPanel.java,
> > 	* controlpanel/org/classpath/controlpanel/ComboItem.java,
> > 	* controlpanel/org/classpath/controlpanel/ControlPanel.java,
> > 	* controlpanel/org/classpath/controlpanel/DebuggingPanel.java,
> > 	*
> controlpanel/org/classpath/controlpanel/DesktopShortcutPanel.java,
> > 	* controlpanel/org/classpath/controlpanel/JREPanel.java,
> > 	* controlpanel/org/classpath/controlpanel/NamedBorderPanel.java,
> > 	*
> controlpanel/org/classpath/controlpanel/SecuritySettingsPanel.java,
> > 	*
> controlpanel/org/classpath/controlpanel/TemporaryInternetFilesPanel.java,
> > 	*
> controlpanel/org/classpath/controlpanel/network/AdvancedProxySettingsDialog.java,
> > 	*
> controlpanel/org/classpath/controlpanel/network/AdvancedProxySettingsPane.java,
> > 	*
> controlpanel/org/classpath/controlpanel/network/NetworkSettingsPanel.java,
> > 	*
> controlpanel/org/classpath/controlpanel/security/viewer/CertificatePane.java,
> > 	*
> controlpanel/org/classpath/controlpanel/security/viewer/CertificateViewer.java,
> > 	*
> controlpanel/org/classpath/controlpanel/translator/Translator.java:
> > 	New classes. All methods are new as well.
> > 	* netx/net/sourceforge/jnlp/resources/Messages.properties: Added
> > 	messages for control panel.
> >
> > Comments? Question? Concerns?
> >
> 
> A few quick thoughts are listed below. Please dont let any of that 
> discourage you. These are all thing that should be improved. But what
> 
> you have so far is great work. It's just not perfect yet ;) We really
> 
> needed something like this in netx/plugin though. Thanks for doing
> this!
> 
> 1. Some of these settings dont do anything yet, right? I am looking at
> 
> the cache stuff here. Perhaps you might want to leave it out for now,
> or 
> clearly indicate that it does not do anything.
Ah, yes. I will put on the description noting that it currently doesn't do anything but just sets the deployment.properties file.

> 
> 2. Why does the binary have the name controlpanel? I dont really care
> 
> about the name itself, but if you are going to make it so close to the
> 
> one used by proprietary JDK, why not just call it ControlPanel?
The proprietary JDK actually calls it jcontrol then symlinked to it calling ControlPanel :)

> 
> 3. We should look into adding a desktop file so users would be able to
> 
> launch it from the menu.
> 
> 4. It is customary to have the package names be representative of the
> 
> url. IcedTea is under the url icedtea.classpath.org. So itt would make
> a 
> lot more sense to use org.classpath.icedtea.* rather than
> org.classpath.*.
I'll change this for the updated patch later.

> 
> 5. Please get rid of CertificateViewer. CertificatePanel should be
> moved 
> to org.classpath.controlpanel, along with the rest of the classes.
Ah yes, I completely forgot about CertificateViewer not being used. 
Move will also be done.

> 
> 6. Please dont create copies of classes. There is already a translator
> 
> class in netx. The new Translator class still has a dependency on 
> JNLPRuntime, so I really dont see the point of duplicating it. 
> Personally I think it would be best if we separate the user-visible 
> strings in ControlPanel from those in netx. We should have a separate
> 
> properties file in control panel. Then the translator class would not
> be 
> an exact duplicate (since it would read strings from another
> location).
> 
> 7. Typo in the patch to ChangeLog:
> +	(load): Do a security check at start. * security exception
Oops, replaced wrong text! I'll fix this.

> 
> 8. Nice work on the launcher! The -Xbootclasspath way avoids adding 
> redundant things to boot classpath.
> 
> 9. I see that some files list asu as the author. Please use your full
> 
> name (and email address) instead.
Ok. =)

> 
> 10. You know you can do static imports right? Something like
> import static org.classpath.controlpanel.translator.Translator.R
> would mean you can just do R("foo") instead of Translator.R everytime.
> 
> This is more of a style thing, so feel free to ignore it.
> 
> Anyway, this looks great as a start! Thanks again for doing this.
> 
> Cheers,
> Omair
Thanks for the review too Omair!

Cheers,
  Andrew