Reviewer needed - fix for Re: Strange behaviour during javaws -about
Dr Andrew John Hughes
ahughes at redhat.com
Thu Feb 24 10:44:53 PST 2011
On 12:52 Thu 24 Feb , Omair Majid wrote:
> Hi,
>
> I am attaching a patch that makes net.sourceforge.jnlp.runtime.Boot use
> the location of about.jnlp as determined at build time. There are a few
> other things that I will be adding to build.properties.
>
> The rest of this email assumes this patch is applied before applying
> your patch, and where there are conflicts (such as the codebase of
> about.jnlp, this patch takes precedence).
>
snip...
>
> >> @@ -112,24 +105,8 @@
> >> }
> >>
> >> public static void main(String[] args) {
> >> - javax.swing.SwingUtilities.invokeLater(new Runnable() {
> >> - public void run() {
> >> - createAndShowGUI();
> >> - }
> >> - });
> >> - }
> >> + createAndShowGUI();
> >> + }
> >>
>
> I dont agree with this change. createAndShowGUI() create a JFrame;
> running this code on the main thread is a violation of the Swing
> threading guidelines [1]. Not saying that there's no bug here (I haven't
> really looked) but the removal of invokeLater looks wrong.
>
Looks wrong to me too, I saw the code removal but not this addition.
This will stop the main thread exiting AFAICS.
> >> diff -r d7fee305bd4f -r e2f1a23ade09
> >> netx/net/sourceforge/jnlp/resources/about.jnlp
> >> --- a/netx/net/sourceforge/jnlp/resources/about.jnlp Wed Feb 23 13:37:10
> >> 2011 -0500
> >> +++ b/netx/net/sourceforge/jnlp/resources/about.jnlp Thu Feb 24 17:33:23
> >> 2011 +0100
> >> @@ -1,5 +1,5 @@
> >> <?xml version="1.0" encoding="utf-8"?>
> >> -<jnlp spec="1.0" href="about.jnlp"
> >> codebase="http://icedtea.classpath.org/netx/">
> >> +<jnlp spec="1.0" href="about.jnlp"
> >> codebase="file:///home/jvanek/icedtea-web-image/share/icedtea-web/">
> >> <information>
> >> <title>About window for NetX</title>
> >> <vendor>NetX</vendor>
>
> Well, if you expect this code to be fixed by someone else's (not posted)
> patch, please dont change it :) It makes patches conflict.
>
> >> @@ -12,7 +12,7 @@
> >> <jar href="about.jar"/>
> >> </resources>
> >> <security>
> >> -<all-permissions/>
> >> +
> >> </security>
> >> <application-desc main-class="net.sourceforge.jnlp.about.Main">
> >> </application-desc>
>
> Again, removing all-permissions and accessing the net.sourceforge.jnlp
> package is currently not possible :(
>
> >> @@ -218,8 +223,16 @@
> >> cl = ClassLoader.getSystemClassLoader();
> >> }
> >> try {
> >> - return
> >> cl.getResource("net/sourceforge/jnlp/resources/about.jnlp").toString();
> >> + //exctracts full path to about.jnlp
> >
> > Typo; extracts
> >
> >> + String s =
> >> cl.getResource("net/sourceforge/jnlp/runtime/Boot.class").toString();
> >> + s=s.substring(0,s.indexOf("!"));
> >> + s=s.substring(s.indexOf(":")+1);
> >> + s=s.substring(s.indexOf(":")+1);
> >> + s="file://"+s.replace("netx.jar","about.jnlp");
> >> + System.out.println(s);
> >> + return s;
> >
> > What does this do?
> >
>
> If I understand this properly, it uses the installed location of
> netx.jar to compute the location of about.jnlp. That is, if netx.jar is
> at /usr/local/share/icedtea-web/netx.jar, then it finds about.jnlp to be
> located at /usr/local/share/icedtea-web/about.jnlp.
>
It's the use of ':' and '!' that confuses me. Exactly what does cl.getResource
return here?
> The attached patch determines the paths at build time and puts them in a
> build.properties file.
>
I'd prefer that approach.
> Cheers,
> Omair
>
> [1]
> http://download.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html
> diff -r d7fee305bd4f Makefile.am
> --- a/Makefile.am Wed Feb 23 13:37:10 2011 -0500
> +++ b/Makefile.am Thu Feb 24 12:06:16 2011 -0500
> @@ -242,7 +242,11 @@
> netx-source-files.txt:
> find $(NETX_SRCDIR) -name '*.java' | sort > $@
>
> -stamps/netx.stamp: netx-source-files.txt stamps/bootstrap-directory.stamp
> +build.properties: $(NETX_SRCDIR)/net/sourceforge/jnlp/build.properties.in
> + sed "s#[@]ABOUT_JNLP_LOCATION[@]#$(DESTDIR)$(datadir)/icedtea-web/about.jnlp#" < $< > $@
Is there a reason we aren't doing this via configure?
$(DESTDIR) will not be the final install location so should be in here.
It's a staging location.
> +
> +stamps/netx.stamp: netx-source-files.txt stamps/bootstrap-directory.stamp \
> + build.properties
> mkdir -p $(NETX_DIR)
> $(BOOT_DIR)/bin/javac $(IT_JAVACFLAGS) \
> -d $(NETX_DIR) \
> @@ -255,6 +259,7 @@
> ${INSTALL_DATA} -D $${files} \
> $(NETX_DIR)/net/sourceforge/jnlp/resources/$${files}; \
> done)
> + cp -pPR build.properties $(NETX_DIR)/net/sourceforge/jnlp/
Any reason not to just use cp -a?
> mkdir -p stamps
> touch $@
>
> @@ -274,6 +279,7 @@
>
> clean-netx:
> rm -rf $(NETX_DIR)
> + rm -f build.properties
configure would do this for you.
> rm -f stamps/netx-dist.stamp
> rm -f netx-source-files.txt
> rm -f stamps/netx.stamp
> diff -r d7fee305bd4f netx/net/sourceforge/jnlp/build.properties.in
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/build.properties.in Thu Feb 24 12:06:16 2011 -0500
> @@ -0,0 +1,1 @@
> +about.jnlp.location=@ABOUT_JNLP_LOCATION@
> diff -r d7fee305bd4f netx/net/sourceforge/jnlp/resources/about.jnlp
> --- a/netx/net/sourceforge/jnlp/resources/about.jnlp Wed Feb 23 13:37:10 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/resources/about.jnlp Thu Feb 24 12:06:16 2011 -0500
> @@ -1,5 +1,5 @@
> <?xml version="1.0" encoding="utf-8"?>
> -<jnlp spec="1.0" href="about.jnlp" codebase="http://icedtea.classpath.org/netx/">
> +<jnlp spec="1.0" href="about.jnlp" codebase=".">
> <information>
> <title>About window for NetX</title>
> <vendor>NetX</vendor>
> diff -r d7fee305bd4f netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java Wed Feb 23 13:37:10 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java Thu Feb 24 12:06:16 2011 -0500
> @@ -19,6 +19,7 @@
> import static net.sourceforge.jnlp.runtime.Translator.R;
>
> import java.io.File;
> +import java.io.InputStream;
> import java.io.IOException;
> import java.net.MalformedURLException;
> import java.net.URL;
> @@ -27,6 +28,7 @@
> import java.util.ArrayList;
> import java.util.Arrays;
> import java.util.List;
> +import java.util.Properties;
>
> import net.sourceforge.jnlp.AppletDesc;
> import net.sourceforge.jnlp.ApplicationDesc;
> @@ -213,15 +215,31 @@
> * does not exist.
> */
> private static String getAboutFile() {
> + String location = null;
> ClassLoader cl = Boot.class.getClassLoader();
> if (cl == null) {
> cl = ClassLoader.getSystemClassLoader();
> }
> + InputStream in = cl.getResourceAsStream("net/sourceforge/jnlp/build.properties");
> try {
> - return cl.getResource("net/sourceforge/jnlp/resources/about.jnlp").toString();
> - } catch (Exception e) {
> - return null;
> + Properties buildProperties = new Properties();
> + buildProperties.load(in);
> + location = buildProperties.getProperty("about.jnlp.location");
> + } catch (IOException e) {
> + // ignore and return null later on
> + if (JNLPRuntime.isDebug()) {
> + e.printStackTrace();
> + }
> + } finally {
> + if (in != null) {
> + try {
> + in.close();
> + } catch (IOException e) {
> + e.printStackTrace();
> + }
> + }
> }
> + return location;
> }
>
> /**
Code changes look fine.
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list