Reviewer needed - fix for Re: Strange behaviour during javaws -about
Dr Andrew John Hughes
ahughes at redhat.com
Thu Feb 24 10:38:01 PST 2011
On 18:20 Thu 24 Feb , Jiri Vanek wrote:
> On 02/24/2011 06:04 PM, Dr Andrew John Hughes wrote:
> > On 17:43 Thu 24 Feb , Jiri Vanek wrote:
> >> On 02/24/2011 12:36 AM, Omair Majid wrote:
> >>
> >> So, this patch expects fixed about.jnlp to have code base of
> >> file://$DEST_DIR/share
> >>
> >> Then removes all-permissions, and lunched fixed window without them. The
> >> window is not running in invoke later(which probably caused the hang)
> >> but runs in javaws's own thread. Also points directly to correct jnlp
> >> local file and runs offline.
> >>
> >
> > Comments inline.
> >
> > Please separate any formatting changes to another patch, though I don't
> > see why these are necessary at all.
> >
>
> All the formating chanegs are {} behind if. Their's existence protect
> from stupid mistake.
>
Even if it's valid, it should be a separate patch.
> >>> On 02/23/2011 12:51 PM, Dr Andrew John Hughes wrote:
> >>>> What is the motivation for this change? Assuming it still runs without
> >>>> all-permissions, the change looks fine.
> >>>>
> >>>
> >>> From my very limited testing, it doesnt :(
> >> >
> >>> snip
> >>
> >> # HG changeset patch
> >> # User Jiri Vanek<jvanek at redhat.com>
> >> # Date 1298565203 -3600
> >> # Node ID e2f1a23ade0931e1b6810cb06d4b313ee9333415
> >> # Parent d7fee305bd4f63d3a37cfa041b9af726e296d22f
> >> about.jar can now be lunched localy, without all-permissions, and
> >> about.jnlp is red from directory instead from jar.
> >>
> >> diff -r d7fee305bd4f -r e2f1a23ade09 ChangeLog
> >> --- a/ChangeLog Wed Feb 23 13:37:10 2011 -0500
> >> +++ b/ChangeLog Thu Feb 24 17:33:23 2011 +0100
> >> @@ -1,3 +1,11 @@
> >> +2011-02-24 Jiri Vanek<jvanek at redhat.com>
> >> +
> >> + * netx/net/sourceforge/jnlp/runtime/Boot.java: getAboutFile changed to
> >> return path to local about.jnlp instead of inner-ftom jar
> >
> > What does ftom stand for?
>
> about.jnlp was laoded from inside from jar. No it is loaded from
> directory where netx.jar is placed.
So 'inner-ftom' should be 'inside the' :-)
> >
> >> + * extras/net/sourceforge/jnlp/about/Main.java: removed ivokelater,
> >> useless hypertextlistener and runtime which needed special permissions.
> >
> > Why is it useless?
> >
>
> Without permissions, the likns can not be lunched by clicking. Most of
> the likns are on second tab with demos, which should be removed from my
> point of view... Eh bed words maybe. Runtime is useless without
> hyperlinklistener and without invoke later, but it itself needed special
> permissions should it have to be removed.
>
If it breaks functionality, why are we removing the permissions?
We should keep the demo tab IMHO.
> > Both this and the above need to be more verbose to include the changes made.
> >
> >> + * netx/net/sourceforge/jnlp/resources/about.jnlp: removed
> >> <all-permissions>
> >> +
> >> +
> >> +
> >> 2011-02-23 Omair Majid<omajid at redhat.com>
> >>
> >> * Makefile.am: Add missing slash to JRE.
> >> diff -r d7fee305bd4f -r e2f1a23ade09
> >> extra/net/sourceforge/jnlp/about/Main.java
> >> --- a/extra/net/sourceforge/jnlp/about/Main.java Wed Feb 23 13:37:10
> >> 2011 -0500
> >> +++ b/extra/net/sourceforge/jnlp/about/Main.java Thu Feb 24 17:33:23
> >> 2011 +0100
> >> @@ -41,19 +41,15 @@
> >> import java.awt.Dimension;
> >> import java.awt.Toolkit;
> >> import java.io.IOException;
> >> -import java.net.URL;
> >> +
> >>
> >> import javax.swing.JFrame;
> >> import javax.swing.JPanel;
> >> import javax.swing.JTabbedPane;
> >> import javax.swing.UIManager;
> >> -import javax.swing.event.HyperlinkEvent;
> >> -import javax.swing.event.HyperlinkListener;
> >>
> >> -import net.sourceforge.jnlp.Launcher;
> >> -import net.sourceforge.jnlp.runtime.JNLPRuntime;
> >>
> >> -public class Main extends JPanel implements HyperlinkListener {
> >> +public class Main extends JPanel{
> >>
> >
> > Why these changes?
> >
>
> as explained^^
> >> private final String notes =
> >> "/net/sourceforge/jnlp/about/resources/notes.html";
> >> private final String apps =
> >> "/net/sourceforge/jnlp/about/resources/applications.html";
> >> @@ -62,13 +58,11 @@
> >>
> >> public Main() throws IOException {
> >> super(new BorderLayout());
> >> -
> >> +
> >> HTMLPanel notesPanel = new HTMLPanel(getClass().getResource(notes));
> >> HTMLPanel appsPanel = new HTMLPanel(getClass().getResource(apps));
> >> HTMLPanel aboutPanel = new HTMLPanel(getClass().getResource(about));
> >>
> >> - appsPanel.pane.addHyperlinkListener(this);
> >> -
> >> tabbedPane = new JTabbedPane();
> >>
> >> tabbedPane.add("About NetX", aboutPanel);
> >> @@ -80,7 +74,6 @@
> >> }
> >>
> >> private static void createAndShowGUI() {
> >> - JNLPRuntime.setExitClass(Main.class);
> >>
> >> try {
> >> UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
> >> @@ -112,24 +105,8 @@
> >> }
> >>
> >> public static void main(String[] args) {
> >> - javax.swing.SwingUtilities.invokeLater(new Runnable() {
> >> - public void run() {
> >> - createAndShowGUI();
> >> - }
> >> - });
> >> - }
> >> + createAndShowGUI();
> >> + }
> >>
> >> - public void hyperlinkUpdate(HyperlinkEvent e) {
> >> - if (e.getEventType() == HyperlinkEvent.EventType.ACTIVATED) {
> >> - URL url = e.getURL();
> >>
> >> - Launcher launcher = new Launcher(
> >> - JNLPRuntime.getDefaultLaunchHandler());
> >> - try {
> >> - launcher.launchBackground(url);
> >> - } catch (Exception ex) {
> >> - ex.printStackTrace();
> >> - }
> >> - }
> >> - }
> >> }
> >> 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>
> >> @@ -12,7 +12,7 @@
> >> <jar href="about.jar"/>
> >> </resources>
> >> <security>
> >> -<all-permissions/>
> >> +
> >> </security>
> >> <application-desc main-class="net.sourceforge.jnlp.about.Main">
> >> </application-desc>
> >> diff -r d7fee305bd4f -r e2f1a23ade09
> >> 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 17:33:23
> >> 2011 +0100
> >> @@ -141,22 +141,26 @@
> >> System.exit(0);
> >> }
> >>
> >> - if (null != getOption("-about"))
> >> + if (null != getOption("-about")) {
> >> System.out.println(aboutMessage);
> >> + }
> >>
> >> - if (null != getOption("-verbose"))
> >> + if (null != getOption("-verbose")) {
> >> JNLPRuntime.setDebug(true);
> >> + }
> >>
> >
> > I don't see the point in these changes. This is just noise which makes
> > the patch hard to read.
> >
> I really hate missing brackets behind if() structure. SO I have added
> them. As a cosmetic change it should be another patch.
Yes, the main issue is: not in this patch :-)
> >> if (null != getOption("-update")) {
> >> int value = Integer.parseInt(getOption("-update"));
> >> JNLPRuntime.setDefaultUpdatePolicy(new UpdatePolicy(value
> >> * 1000l));
> >> }
> >>
> >> - if (null != getOption("-headless"))
> >> + if (null != getOption("-headless")) {
> >> JNLPRuntime.setHeadless(true);
> >> + }
> >>
> >> - if (null != getOption("-noupdate"))
> >> + if (null != getOption("-noupdate")) {
> >> JNLPRuntime.setDefaultUpdatePolicy(UpdatePolicy.NEVER);
> >> + }
> >>
> >> if (null != getOption("-Xnofork")) {
> >> JNLPRuntime.setForksAllowed(false);
> >> @@ -194,8 +198,9 @@
> >> } catch (LaunchException ex) {
> >> // default handler prints this
> >> } catch (Exception ex) {
> >> - if (JNLPRuntime.isDebug())
> >> + if (JNLPRuntime.isDebug()) {
> >> ex.printStackTrace();
> >> + }
> >>
> >> fatalError(R("RUnexpected", ex.toString(),
> >> ex.getStackTrace()[0]));
> >> }
> >> @@ -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?
>
>
> This extracts full path to netx.jar (and from it) to about.jnlp file.
> I do not know different way ho to mine this information.
I get the first line. What does all the substitution do? And should we
be printing to stdout?
>
> >
> >> } catch (Exception e) {
> >> + e.printStackTrace();
> >> return null;
> >> }
> >> }
> >> @@ -235,8 +248,9 @@
> >> // override -jnlp with aboutFile
> >> if (getOption("-about") != null) {
> >> location = getAboutFile();
> >> - if (location == null)
> >> + if (location == null) {
> >> fatalError(R("RNoAboutJnlp"));
> >> + }
> >> } else {
> >> location = getJNLPFile();
> >> }
> >> @@ -246,21 +260,24 @@
> >> System.exit(1);
> >> }
> >>
> >> - if (JNLPRuntime.isDebug())
> >> + if (JNLPRuntime.isDebug()) {
> >> System.out.println(R("BFileLoc") + ": " + location);
> >> + }
> >>
> >> URL url = null;
> >>
> >> try {
> >> - if (new File(location).exists())
> >> - // TODO: Should be toURI().toURL()
> >> + if (new File(location).exists()) // TODO: Should be
> >> toURI().toURL()
> >> + {
> >> url = new File(location).toURL(); // Why use
> >> file.getCanonicalFile?
> >> - else
> >> + } else {
> >> url = new
> >> URL(ServiceUtil.getBasicService().getCodeBase(), location);
> >> + }
> >> } catch (Exception e) {
> >> fatalError("Invalid jnlp file " + location);
> >> - if (JNLPRuntime.isDebug())
> >> + if (JNLPRuntime.isDebug()) {
> >> e.printStackTrace();
> >> + }
> >> }
> >>
> >> boolean strict = (null != getOption("-strict"));
> >> @@ -275,20 +292,26 @@
> >> // add in extra params from command line
> >> addProperties(file);
> >>
> >> - if (file.isApplet())
> snip
>
> (hope that I have not overlooked comment)
Just the one about ChangeLog verbosity ;-)
>
> Regards J.
--
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