Icedtea-web splashscreen implementation
Omair Majid
omajid at redhat.com
Thu Mar 22 11:21:00 PDT 2012
Hi Jiri,
On 03/02/2012 07:35 AM, Jiri Vanek wrote:
> splasScreen-integration.diff
I have looked over this patch and posted my thoughts below.
> 2012-03-02 Jiri Vanek <jvanek at redhat.com>
>
> Integrated splashscreen, added authors, added bitmap splash,
> * AUTHORS: added Jan Kmetko as the person behind initial bitmap
> splash design
I am not sure what's the requirement for adding an author; can we have
an author who did not provide code?
> * Makefile.am: (edit_launcher_script) enriched for replacement
> JAVAWS_SPLASH_LOCATION inside javaws.in by real
> datadir/PACKAGE_NAME/javaws_splash.png value
> (install-exec-local) enriched for installing javaws_splash.png from
> NETX_SRCDIR to datadir/PACKAGE_NAME
IcedTea-Web contains a number of images already; they are under
netx/net/sourceforge/jnlp/resources. Any reason you want to have this
image outside of netx.jar? If we leave it in netx.jar, we wont have to
do these modifications to the makefiles.
> *NEWS: mentioned splash screen
> * extra/net/sourceforge/javaws/about/resources/notes.html: added
> Danesh, Saad, me and Jan as missing developers and artist
Please post a separate patch to add Danesh and Saad to the authors list;
don't do it here.
> * netx/net/sourceforge/jnlp/GuiLaunchHandler.java: implemented
> missing default splash screen with vector spalsh and with information's
> element content
> * netx/net/sourceforge/jnlp/JNLPSplashScreen.java: fixed to be able
> to use custom image or (if not defined or broken) our vector one
> * netx/net/sourceforge/jnlp/Launcher.java: added hooks which will
> force applet to show error screen in case of error
> change on lines @@ -696,22 +705,30 @@ probably does not work, and
> it is the case when connection with server is interrupted in middle of
> downloading jar
> (createApplet) and (createAppletObject) were made synchronized to
> prevent several applets lunched in one time to touch each other
> * netx/net/sourceforge/jnlp/NetxPanel.java: added field, getter and
> setter for its XEmbededFrame
> * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java: main
> (createPanel)'s run is adding splash screen after start of
> onitialization and removing it before init.
> (framePanel) returns PluginAppletViewer and is creating sdplash
> (removeSplash) new method to handle spalsh removing
> (showErrorSplash) new method to be called by hooks to swap splash to
> error state
> * netx/net/sourceforge/jnlp/runtime/Boot.java): (name) and (version)
> made public
More comments below, in-line.
> diff -r f058f21b9e99 launcher/javaws.in
> --- a/launcher/javaws.in Wed Feb 22 10:18:45 2012 -0500
> +++ b/launcher/javaws.in Thu Mar 01 13:31:17 2012 +0100
> @@ -5,6 +5,7 @@
> LAUNCHER_FLAGS=-Xms8m
> CLASSNAME=net.sourceforge.jnlp.runtime.Boot
> BINARY_LOCATION=@JAVAWS_BIN_LOCATION@
> +SPLASH_LOCATION=@JAVAWS_SPLASH_LOCATION@
> PROGRAM_NAME=javaws
> CP=@JRE@/lib/rt.jar
>
> @@ -15,6 +16,10 @@
> i=0
> j=0
>
> +SPLASH="false"
> +if [ "x$ICEDTEA_WEB_SPLASH" = "x" ] ; then
> +SPLASH="true"
> +fi;
> while [ "$#" -gt "0" ]; do
> case "$1" in
> -J*)
> @@ -24,6 +29,9 @@
> *)
> ARGS[$j]="$1"
> j=$((j+1))
> + if [ "$1" = "-headless" ] ; then
> + SPLASH="false"
> + fi
Are you sure this is actually needed? If we only show the splash screen
(in javaws) using the GuiLaunchhandler, and that isn't plugged-in in
headless mode, then we don't need there here.
> ;;
> esac
> shift
> @@ -32,6 +40,10 @@
> k=0
> COMMAND[k]="${JAVA}"
> k=$((k+1))
> +if [ "$SPLASH" = "true" ] ; then
> +COMMAND[k]="-splash:${SPLASH_LOCATION}"
> +k=$((k+1))
> +fi;
Hm... so we just show a plain image and then later replace it with a
dynamic one?
> COMMAND[k]="${LAUNCHER_BOOTCLASSPATH}"
> k=$((k+1))
> COMMAND[k]="${LAUNCHER_FLAGS}"
> diff -r f058f21b9e99 netx/javaws_splash.png
> Binary file netx/javaws_splash.png has changed
> diff -r f058f21b9e99 netx/net/sourceforge/jnlp/GuiLaunchHandler.java
> --- a/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Wed Feb 22 10:18:45 2012 -0500
> +++ b/netx/net/sourceforge/jnlp/GuiLaunchHandler.java Thu Mar 01 13:31:17 2012 +0100
> @@ -1,5 +1,5 @@
Currently, the launcher handler is for the enitre runtime. Perhaps we
should limit it to per Application/Applet?
> /* GuiLaunchHandler.java
> - Copyright (C) 2011 Red Hat, Inc.
> + Copyright (C) 2012 Red Hat, Inc.
>
> This file is part of IcedTea.
>
> @@ -74,10 +74,13 @@
> }
>
> private void closeSplashScreen() {
> - synchronized(mutex) {
> + synchronized (mutex) {
> if (splashScreen != null) {
> if (splashScreen.isSplashScreenValid()) {
> splashScreen.setVisible(false);
> + if (splashScreen.isComponented()) {
> + splashScreen.stopSpining();
> + }
> }
> splashScreen.dispose();
> }
> @@ -96,40 +99,42 @@
>
> @Override
> public void launchInitialized(final JNLPFile file) {
> -
> +
> int preferredWidth = 500;
> int preferredHeight = 400;
>
> final URL splashImageURL = file.getInformation().getIconLocation(
> IconDesc.SPLASH, preferredWidth, preferredHeight);
>
> + final ResourceTracker resourceTracker = new ResourceTracker(true);
> if (splashImageURL != null) {
> - final ResourceTracker resourceTracker = new ResourceTracker(true);
> resourceTracker.addResource(splashImageURL, file.getFileVersion(), null, policy);
> - synchronized(mutex) {
> - try {
> - SwingUtilities.invokeAndWait(new Runnable() {
> - @Override
> - public void run() {
> - splashScreen = new JNLPSplashScreen(resourceTracker, null, null);
> - }
> - });
> - } catch (InterruptedException ie) {
> - // Wait till splash screen is created
> - while (splashScreen == null);
> - } catch (InvocationTargetException ite) {
> - ite.printStackTrace();
> - }
> + }
> + synchronized (mutex) {
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
>
> - splashScreen.setSplashImageURL(splashImageURL);
> + @Override
> + public void run() {
> + splashScreen = new JNLPSplashScreen(resourceTracker, file);
> + }
> + });
> + } catch (InterruptedException ie) {
> + // Wait till splash screen is created
> + while (splashScreen == null);
I see this problem was in the existing code too, but this loop may not
work too well. The variable splashScreen is not volatile, so changes to
it may not be propagated :(
> + } catch (InvocationTargetException ite) {
> + ite.printStackTrace();
> }
> +
> + splashScreen.setSplashImageURL(splashImageURL);
If I am following the code right, this call in turn invokes setLayout()
on this swing container. This should be done in the EDT.
> }
> -
> +
> SwingUtilities.invokeLater(new Runnable() {
> +
> @Override
> public void run() {
> - if (splashImageURL != null) {
> - synchronized(mutex) {
> + if (splashScreen != null) {
> + synchronized (mutex) {
> if (splashScreen.isSplashScreenValid()) {
> splashScreen.setVisible(true);
> }
> diff -r f058f21b9e99 netx/net/sourceforge/jnlp/JNLPSplashScreen.java
> --- a/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Wed Feb 22 10:18:45 2012 -0500
> +++ b/netx/net/sourceforge/jnlp/JNLPSplashScreen.java Thu Mar 01 13:31:17 2012 +0100
> @@ -14,80 +52,107 @@
>
> import net.sourceforge.jnlp.cache.ResourceTracker;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> +import net.sourceforge.jnlp.splashscreen.SplashPanel;
> +import net.sourceforge.jnlp.splashscreen.SplashUtils;
> +import net.sourceforge.jnlp.splashscreen.parts.InformationElement;
>
> public class JNLPSplashScreen extends JDialog {
>
> - String applicationTitle;
> - String applicationVendor;
> +
>
> ResourceTracker resourceTracker;
>
> URL splashImageUrl;
> Image splashImage;
> + private final JNLPFile file;
> + public static final int DEF_W=635;
> + public static final int DEF_H=480;
DEFAULT_WIDTH/DEFAULT_HEIGHT ?
> + private SplashPanel componetSplash;
>
> - public JNLPSplashScreen(ResourceTracker resourceTracker,
> - String applicationTitle, String applicationVendor) {
> + public JNLPSplashScreen(ResourceTracker resourceTracker,final JNLPFile file) {
>
> // 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.
>
> this.resourceTracker = resourceTracker;
> - this.applicationTitle = applicationTitle;
> - this.applicationVendor = applicationVendor;
> + this.file=file;
> +
>
> }
>
> public void setSplashImageURL(URL url) {
> - splashImageUrl = url;
> - splashImage = null;
> - try {
> - splashImage = ImageIO.read(resourceTracker
> - .getCacheFile(splashImageUrl));
> - if (splashImage == null) {
> + if (url != null) {
> + splashImageUrl = url;
> + splashImage = null;
> + try {
> + splashImage = ImageIO.read(resourceTracker.getCacheFile(splashImageUrl));
As an optimization, perhaps we can check if the image is already cached.
If it is cached, then we use it otherwise we cache the image in the
background while displaying the normal splash screen?
> + if (splashImage == null) {
> + if (JNLPRuntime.isDebug()) {
> + System.err.println("Error loading splash image: " + url);
> + }
> +
> + }
> + } catch (IOException e) {
> if (JNLPRuntime.isDebug()) {
> System.err.println("Error loading splash image: " + url);
> }
> - return;
> + splashImage = null;
> +
> + } catch (IllegalArgumentException argumentException) {
> + if (JNLPRuntime.isDebug()) {
> + System.err.println("Error loading splash image: " + url);
> + }
> + splashImage = null;
> +
> }
> - } catch (IOException e) {
> - if (JNLPRuntime.isDebug()) {
> - System.err.println("Error loading splash image: " + url);
> - }
> - splashImage = null;
> - return;
> - } catch (IllegalArgumentException argumentException) {
> - if (JNLPRuntime.isDebug()) {
> - System.err.println("Error loading splash image: " + url);
> - }
> - splashImage = null;
> - return;
> }
>
> + if (splashImage == null) {
> + this.setLayout(new BorderLayout());
> + SplashPanel splash = SplashUtils.getSplashScreen(DEF_W, DEF_H, SplashUtils.SplashReason.JAVAWS);
Does it really need to know a reason?
> + if (splash != null) {
> + splash.spin();
Can we rename this to something more generic? A quick but bad example
would be start()?
> + splash.setInformationContent(InformationElement.createFromJNLP(file));
Can we use the InformationDesc from the jnlp file directly.
> + this.add(splash.getPanel());
SplashPanel is a component that can be added to another swing component?
This I like!
> + this.componetSplash = splash;
> + }
> + }
> correctSize();
> }
>
> public boolean isSplashScreenValid() {
> - return (splashImage != null);
> + return (splashImage != null) || (componetSplash != null);
> +
> }
>
> private void correctSize() {
> + int minimumWidth = DEF_W;
> + int minimumHeight = DEF_H;
> + if (splashImage != null) {
> + Insets insets = getInsets();
> + minimumWidth = splashImage.getWidth(null) + insets.left
> + + insets.right;
> + minimumHeight = splashImage.getHeight(null) + insets.top
> + + insets.bottom;
> + }
> + setMinimumSize(new Dimension(0, 0));
> + setMaximumSize(new Dimension(Integer.MAX_VALUE, Integer.MAX_VALUE));
> + setSize(new Dimension(minimumWidth, minimumHeight));
> + setPreferredSize(new Dimension(minimumWidth, minimumHeight));
>
> - Insets insets = getInsets();
> - int minimumWidth = splashImage.getWidth(null) + insets.left
> - + insets.right;
> - int minimumHeight = splashImage.getHeight(null) + insets.top
> - + insets.bottom;
> - setMinimumSize(new Dimension(minimumWidth, minimumHeight));
> -
> - Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
> - setLocation((screenSize.width - minimumWidth) / 2,
> - (screenSize.height - minimumHeight) / 2);
> + // Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
> + // setLocation((screenSize.width - minimumWidth) / 2,
> + // (screenSize.height - minimumHeight) / 2);
> + //Above works always, but center only to middle of all monitors
> + //below works on 1.4 and higher, and center to middle of primary monmtor
> + setLocationRelativeTo(null);
Does this mean that it will center to the primary monitor instead of the
monitor where javaws was started in?
> }
>
> @Override
> public void paint(Graphics g) {
> if (splashImage == null) {
> + super.paint(g);
> return;
> }
>
> @@ -96,4 +161,12 @@
> g2.drawImage(splashImage, getInsets().left, getInsets().top, null);
>
> }
> +
> + boolean isComponented() {
> + return (componetSplash!=null);
> + }
Whether this container contains components seems to be an implementation
detail. Can you rename this method to make the intention more obvious?
That is, when would I, an external developer, want to call this.
> +
> + void stopSpining() {
> + if (isComponented()) componetSplash.stopSpinning();
> + }
Again, spinning seems to be an implementation detail. What if we change
the splash screen so it doesn't contain anything spinning?
> }
> diff -r f058f21b9e99 netx/net/sourceforge/jnlp/Launcher.java
> --- a/netx/net/sourceforge/jnlp/Launcher.java Wed Feb 22 10:18:45 2012 -0500
> +++ b/netx/net/sourceforge/jnlp/Launcher.java Thu Mar 01 13:31:17 2012 +0100
> @@ -649,18 +651,22 @@
> * @param enableCodeBase whether to add the codebase URL to the classloader
> */
> protected ApplicationInstance launchApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> - if (!file.isApplet())
> + if (!file.isApplet()) {
> throw launchError(new LaunchException(file, null, R("LSFatal"), R("LCClient"), R("LNotApplet"), R("LNotAppletInfo")));
> + }
>
> + AppletInstance applet = null;
> try {
> - AppletInstance applet = createApplet(file, enableCodeBase, cont);
> + applet = createApplet(file, enableCodeBase, cont);
> applet.initialize();
>
> applet.getAppletEnvironment().startApplet(); // this should be a direct call to applet instance
> return applet;
> } catch (LaunchException lex) {
> + SplashUtils.showErrorCaught(lex, applet);
Why not let the launch handler handle this isntead. It will avoid this
tight linking of SplashUtils with Launcher here.
> @@ -742,7 +760,7 @@
> * @param file the PluginBridge to be used.
> * @param enableCodeBase whether to add the code base URL to the classloader.
> */
> - protected Applet createAppletObject(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> + protected synchronized Applet createAppletObject(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
Why the synchronized here?
> diff -r f058f21b9e99 netx/net/sourceforge/jnlp/NetxPanel.java
> --- a/netx/net/sourceforge/jnlp/NetxPanel.java Wed Feb 22 10:18:45 2012 -0500
> +++ b/netx/net/sourceforge/jnlp/NetxPanel.java Thu Mar 01 13:31:17 2012 +0100
> @@ -232,4 +233,13 @@
> SunToolkit.createNewAppContext();
> }
> }
> +
> + public XEmbeddedFrame getAppletViewerFrame() {
> + return appletViewerFrame;
> + }
> +
> + public void setAppletViewerFrame(XEmbeddedFrame appletViewerFrame) {
> + this.appletViewerFrame = appletViewerFrame;
> + }
Do we really need this to be an XEmbeddedFrame? Is an abstraction not
sufficent?
> diff -r f058f21b9e99 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Wed Feb 22 10:18:45 2012 -0500
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Thu Mar 01 13:31:17 2012 +0100
I am afraid of adding more swing dependencies to this class.
I am not too familiar with this class, so I will let someone else (who
is more familiar) look over it for issues with . My main concern is that
we are tightly coupling applet loading code with splash screen code. We
should do this more generically so we can disable/remove/swap out the
splash screen with less hassle. I am considering making launchHandler
per-instance and slightly more generic to handle this sort of thing.
> @@ -517,6 +600,10 @@
> waitForAppletInit(applets.get(identifier).panel);
>
> // Should we proceed with reframing?
> + //appletFrame.removeSplash();
> + //framePanel(identifier, oldFrame.statusMsgStream, handle, applets.get(identifier).panel);
> + System.err.println("Init complete");
> +
The commented out code scares me. Is it needed or not? Why add it?
> if (updateStatus(identifier, PAV_INIT_STATUS.REFRAME_COMPLETE).equals(PAV_INIT_STATUS.INACTIVE)) {
> destroyApplet(identifier);
> return;
> @@ -656,6 +743,8 @@
> */
> public static void waitForAppletInit(NetxPanel panel) {
>
> + System.err.println("Waiting for applet init");
> +
Perhaps you mean plugin.debug() or something?
> +
> + private class PluginAppletPanel extends JPanel {
> + // Nothing to do . Yet..
> + }
>
What's the purpose of this class?
Cheers,
Omair
More information about the distro-pkg-dev
mailing list