Icedtea-web splashscreen implementation

[Omair Majid]

Hi Jiri,

On 07/23/2012 09:53 AM, Jiri Vanek wrote:
> Long pause but here it is. Redesigned as you wished  for head and 1.3
> All minor stuff you have mentioned and most of mayor stuff should been
> fixed.

Thanks for sticking with the patch.

> Minor changes:
>  * IcedTea instead of ICEDtea
>  * no reflection (interfaces instead)
>  * a lot of renaming as suggested
>  * countless;)
> 
> Mayor changes:
>  * heavily unittested
>  * more decomposed
>  * separated SplashScreen and ErrorSplashScrren on highest (component
> and interface) levels as you desired.

Thanks!

> The only "not fixed" is separation of net.sourceforge.jnlp.LaunchHandler
> for each applet. I have trued to separate it somehow cleanly and
> harmlessly but without success. So I would like to proceed without this
> modification (especially for 1.3) and try to figure out more for head.

Hm.. I would have liked to see that.

> I hope that you will like this version more then the previous one;)

Please see my comments in-line below. Also, most of these are hints. So
you can ignore them. If I really feel strongly against something, I will
say so.

> diff -r 01544fb82384 netx/javaws_splash.png
> Binary file netx/javaws_splash.png has changed

Have I mentioned that hg diff --git and mq allow sending hex encoded
binary images?

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/SplashControler.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/SplashControler.java	Mon Jul 23 14:52:44 2012 +0200

> +public interface SplashControler {

Controller (extra l).


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/SplashPanel.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/SplashPanel.java	Mon Jul 23 14:52:44 2012 +0200

> +public interface SplashPanel {
> +
> +    /**
> +     * The plugin splashscreens must be placed into another containers,
> +     * So must return themselves as JComponent.
> +     * Mostly your SplashScreen will extend some JComponent, so this method will
> +     * just return "this"
> +     */
> +    public JComponent getPanel();

Nit: getPanel() makes it sound like the return type is JPanel.

> +    /**
> +     * javaws should provide content of <information> tag. Those informations hould be passed by this method
> +     */
> +    public void setInformationContent(InformationElement content);
> +
> +    public InformationElement getInformationContent();

Sounds like InformationElement should be named InformationContent (or
maybe the methods should be named getInformation/setInformation ?).

> +    /** Width of the plugin window */
> +    public void setPluginWidth(int pluginWidth);
> +
> +    /** Height of the plugin window */
> +    public void setPluginHeight(int pluginHeight);
> +
> +    /** Width of the plugin window */
> +    public int getPluginWidth();
> +
> +    /** Height of the plugin window */
> +    public int getPluginHeight();
> +
> +    public void adjustForSize(int width, int height);

Does the caller have to call setPluginHeight(height),
setPluginWidth(widhth) followed by adjustForSize(width, height) ?
Perhaps adjustForSize can be an internal detail? I really dont think
adjustForSize should be taking extra parameters though.

> +
> +    public void paint(Graphics g);
> +
> +    public void paintComponent(Graphics g);
> +
> +    public void update(Graphics g);

I don't think paint/paintComponent/update to be exposed as public. They
are implementation details, right?

> +    SplashReason getSplashReason();

> +    public String getVersion();

> +    public int getPercentage();

Just a question: do you really need all the getFoo methods? Seems like
the controller will be setting the values, so nothing should have to
query it. Maybe I am mistaken.

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/SplashUtils.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/SplashUtils.java	Mon Jul 23 14:52:44 2012 +0200

> +public class SplashUtils {
> +
> +    static final String ICEDTEA_WEB_PLUGIN_SPLASH = "ICEDTEA_WEB_PLUGIN_SPLASH";
> +    static final String ICEDTEA_WEB_SPLASH = "ICEDTEA_WEB_SPLASH";
> +    static final String NONE = "none";
> +    static final String DEFAULT12 = "default12";

What's DEFUALT12?

> +    public static void showErrorCaught(Throwable ex, AppletInstance appletInstance) {
> +        try {
> +            showError(ex, appletInstance);
> +        } catch (Throwable t) {
> +            if (JNLPRuntime.isDebug()) {
> +                // prinitng this exception is discutable. I have let it in for case that
> +                //some retyping will fail
> +                t.printStackTrace();

For HEAD (not 1.3) maybe consider throwing an InternalError(t) or
RuntimeException(t) instead?

> +    public static void showError(Throwable ex, AppletEnvironment ae) {
> +        if (ae == null) {
> +            return;
> +        }
> +        NetxPanel p = ((NetxPanel) (ae.getAppletFrame()));

Is NetxPanel a SplashController?

> +    private static SplashReason determineCaller(StackTraceElement[] stackTrace) {
> +        for (StackTraceElement stackTraceElement : stackTrace) {
> +            if (stackTraceElement.getClassName().contains(JNLPSplashScreen.class.getSimpleName())) {
> +                return SplashUtils.SplashReason.JAVAWS;
> +            }
> +
> +        }
> +        return SplashUtils.SplashReason.APPLET;
> +    }

Instead of querying the stack, perhaps we can set this from, say,
JNLPRuntime?


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/DefaultErrorSpalshScreen12.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/DefaultErrorSpalshScreen12.java	Mon Jul 23 14:52:44 2012 +0200

> +public final class DefaultErrorSpalshScreen12 extends BasicComponentErrorSplashScreen {

s/Spalsh/Splash/

What's the significance of 12 in the name?

> +
> +    private final DefaultErrorSpalshScreen12 self;
> +    //scaling

Is this a comment for a variable that was removed?

> +    //for clicking ot error message
> +    ErrorPainter painter;

Please make this private.

> +//        try {
> +//            tmpBackround = ImageIO.read(new File("/home/jvanek/Desktop/icedteaplugin.jpg"));
> +//        } catch (Exception ex) {
> +//            ex.printStackTrace();
> +//        }

Please remove this.

> +        // Add a new listener for resizes
> +        addComponentListener(new ComponentListener() {
> +            // Nothing to do for this
> +
> +            @Override
> +            public void componentShown(ComponentEvent e) {
> +            }
> +            // Re-adjust variables based on size
> +
> +            @Override
> +            public void componentResized(ComponentEvent e) {
> +                self.adjustForSize(getWidth(), getHeight());
> +                repaint();
> +            }
> +            // Nothing to do for this
> +
> +            @Override
> +            public void componentMoved(ComponentEvent e) {
> +            }
> +            // Nothing to do for this
> +
> +            @Override
> +            public void componentHidden(ComponentEvent e) {
> +            }
> +        });

You may be able to use ComponentAdapter to cut down the size of this code.

> +    @Override
> +    public void paintComponent(Graphics g) {
> +        paint(g);
> +    }

> +
> +    @Override
> +    public void paint(Graphics g) {
> +        painter.paint(g);


I think just paintComponent should be overridden. Swing's implementation
of paint will call paintComponent at the right time.

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/DefaultSpalshScreen12.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/DefaultSpalshScreen12.java	Mon Jul 23 14:52:44 2012 +0200
> @@ -0,0 +1,155 @@

> +public final class DefaultSpalshScreen12 extends BasicComponentSplashScreen {

This class looks a lot like the last one. There's not much too much code
here. Perhaps you can refactor it into a shared class or something?

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/BasePainter.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/BasePainter.java	Mon Jul 23 14:52:44 2012 +0200
> @@ -0,0 +1,511 @@

> +public class BasePainter {
> +
> +    protected final BasicComponentSplashScreen master;
> +    //animations
> +    //level of water (0-100%)
> +    private int level = 0;

Maybe just call this variable waterLevel? Although it would be even
better to call it something that is more semantically meaningful. What
does that water level reflect?

> +    //waving of water and position of shhadowed WEB
> +    private int level2 = 0;
> +    private int l2inc = 15; //how quickly is greyed web moving

Likewise ;)

> +    //inidivdual sizes, all converging to ZERO!!
> +    private final int WEB_TOP_ALIGMENT = 324;
> +    private final int WEB_LEFT_ALIGMENT = 84;

How did you drive these numbers.

> +    public final void startAnimationThreads() {
> +        Thread tt = getMoovingTextThread();
> +        tt.start();
> +        Thread t = getWatterLevelThread();
> +        t.start();
> +    }

> +
> +    private Thread getMoovingTextThread() {
> +        Thread tt = new Thread() {
> +
> +            @Override
> +            public void run() {
> +                // While spinning
> +                while (master.isAnimationRunning()) {
> +                    try {
> +                        SwingUtilities.invokeAndWait(new Runnable() {
> +
> +                            @Override
> +                            public void run() {
> +                                level2 += l2inc;
> +                                if (level2 > 10000) {
> +                                    level2 = 1;
> +                                }
> +                                // Force repaint
> +                                master.repaint();
> +                            }
> +                        });
> +                        Thread.sleep(150);
> +                    } catch (Exception e) {
> +                        e.printStackTrace();
> +                    }
> +                }
> +            }
> +        };
> +        tt.setDaemon(true);
> +        return tt;
> +    }

My first instic would be to have the animator thread in the controller.
The controller would signal when it's time for the view to update, and
the view would repaint itself. What shuts this thread down?

> +    public void cleareCachedWatter() {
> +        oldTwl = null;
> +    }

clearCachedWater, I think. But I would encourage you to go with a more
genric name (the implementation will be the same, of course). Maybe
something like reset() or clear() ?

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ControlCurve.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ControlCurve.java	Mon Jul 23 14:52:44 2012 +0200
> @@ -0,0 +1,184 @@

> +/** This class represents a curve defined by a sequence of control points */
> +/*  Part of NatCubic implementation, inspire by http://www.cse.unsw.edu.au/~lambert/*/

Could you clarify this bit? Is this code copied from NatCubic?

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ErrorPainter.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ErrorPainter.java	Mon Jul 23 14:52:44 2012 +0200

> +    private Thread getErrorScreamThread() {
> +        // Create a new thread to screem error  when failure

I am not sure I follow. Could you explain the purpose?


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ImageFontCutter.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/ImageFontCutter.java	Mon Jul 23 14:52:44 2012 +0200

> +public class ImageFontCutter {

>From what I can tell, this is supposed to draw strings. Why the font
cutter name? Maybe something like TextRenderer might be more
appropriate? With a method named render:
render (Image image, String text, Font font) { .... }

Just a thought.

> +    /**
> +     * @return the s
> +     */
> +    public String getS() {

Does 's' have a special meaning here?

> +}
> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/MoovingText.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/MoovingText.java	Mon Jul 23 14:52:44 2012 +0200

> +public class MoovingText extends TextWithWatterLevel {

MovingText. TextWithWaterLevel.


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/NatCubic.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/impls/defaultspalshscreen12/NatCubic.java	Mon Jul 23 14:52:44 2012 +0200

I am guessing this is from NatCubic as well?


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/DescriptionInfoItem.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/DescriptionInfoItem.java	Mon Jul 23 14:52:44 2012 +0200

> +/**
> + *description element: A short statement about the application. Description elements are optional. The kind attribute defines how the description should be used. It can have one of the following values:
> + *
> + *   * one-line: If a reference to the application is going to appear on one row in a list or a table, this description will be used.
> + *   * short: If a reference to the application is going to be displayed in a situation where there is room for a paragraph, this description is used.
> + *   * tooltip: If a reference to the application is going to appear in a tooltip, this description is used.
> + *
> + * Only one description element of each kind can be specified. A description element without a kind is used as a default value. Thus, if Java Web Start needs a description of kind short, and it is not specified in the JNLP file, then the text from the description without an attribute is used.
> + *
> + * All descriptions contain plain text. No formatting, such as with HTML tags, is supported.
> + */

Please limit to 80 columns!

> +    @Override
> +    public String toXml() {
> +        if (kind==null){
> +            return super.toXml();
> +        }
> +        return "<"+type+" kind=\""+kind+"\">"+value+"</"+type+">";
> +    }

Hm.. what's this for? Do we ever output anything in XML?


> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/InfoItem.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/InfoItem.java	Mon Jul 23 14:52:44 2012 +0200

> +
> +/**
> + *The optional kind="splash" attribute may be used in an icon element to indicate that the image is to be used as a "splash" screen during the launch of an application. If the JNLP file does not contain an icon element with kind="splash" attribute, Java Web Start will construct a splash screen using other items from the information Element.
> + *If the JNLP file does not contain any icon images, the splash image will consist of the application's title and vendor, as taken from the JNLP file.
> + *
> + * items not used inside
> + */
> +public class InfoItem {

Much of this is more-or-less an exact duplicate of existing code. I know
you want to avoid mixing parsing code with code for splash screen, but
please consider reducing the duplication. As it is, if an additional
element is added to the jnlp file, multiple locations will have to be
changed. This is a bad idea from a maintenance point of view. It is
likely this code will start to rot.

> diff -r 01544fb82384 netx/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialog.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/splashscreen/parts/JEditorPaneBasedExceptionDialog.java	Mon Jul 23 14:52:44 2012 +0200

> +public class JEditorPaneBasedExceptionDialog extends javax.swing.JDialog implements HyperlinkListener {

A screenshot of this might be nice (but not needed). It's hard for me to
'see' what this looks like.

Anyway, my recommendation is to use JOptionPane's dialog if at all possible.

> +    @SuppressWarnings("unchecked")
> +    private void initComponents() {
> +
> +        jPanel2 = new javax.swing.JPanel();
> +        jButton1 = new javax.swing.JButton();
> +        jButton2 = new javax.swing.JButton();
> +        jPanel1 = new javax.swing.JPanel();
> +        jLabel1 = new javax.swing.JLabel();
> +        jLabel2 = new javax.swing.JLabel();
> +        jPanel3 = new javax.swing.JPanel();
> +        jScrollPane1 = new javax.swing.JScrollPane();
> +        jEditorPane1 = new javax.swing.JEditorPane();
> +        jButton3 = new javax.swing.JButton();

Can we have some cleaner names, please?

Also, this looks like some terrible ide-generated code :(

> +    private void jButton2ActionPerformed(java.awt.event.ActionEvent evt) {
> +        if (exception != null) {
> +            try {
> +                StringSelection data = new StringSelection(getExceptionStackTraceAsString(exception));
> +                Clipboard clipboard = Toolkit.getDefaultToolkit().getSystemClipboard();
> +                clipboard.setContents(data, data);

Hm... is this code running with full privileges? Can it access the
clipboard?

> +            } catch (Exception ex) {
> +                JOptionPane.showMessageDialog(this, Translator.R(InfoItem.SPLASH + "cantCopyEx"));
> +                ex.printStackTrace();
> +            }
> +        } else {
> +            JOptionPane.showMessageDialog(this, Translator.R(InfoItem.SPLASH + "noExRecorded"));
> +        }
> +        close();
> +    }
> +
> +    private void jButton3ActionPerformed(java.awt.event.ActionEvent evt) {
> +        // TODO add your handling code here
> +        jEditorPane1.setText(message);
> +        jButton3.setVisible(false);
> +    }
> +
> +    private void jButton1ActionPerformed(java.awt.event.ActionEvent evt) {
> +        // TODO add your handling code here:
> +        close();
> +    }

Could you clarify the method names? Also, please remove the spurious TODOs.

> +    // Variables declaration - do not modify
> +    private javax.swing.JButton jButton1;
> +    private javax.swing.JButton jButton2;
> +    private javax.swing.JButton jButton3;
> +    private javax.swing.JEditorPane jEditorPane1;
> +    private javax.swing.JLabel jLabel1;
> +    private javax.swing.JLabel jLabel2;
> +    private javax.swing.JPanel jPanel1;
> +    private javax.swing.JPanel jPanel2;
> +    private javax.swing.JPanel jPanel3;
> +    private javax.swing.JScrollPane jScrollPane1;
> +    // End of variables declaration

/me mumbles something about IDEs presuming too much.

But seriously, what happens if I do modify this? Say I have to remove a
button to resolve a (usability) bug. Will it completely break things for
your IDE?

I am going to review the other patches later. Sorry to leave you hanging
like this.

Cheers,
Omair