Icedtea-web splashscreen implementation
Jiri Vanek
jvanek at redhat.com
Tue Mar 27 03:36:51 PDT 2012
On 03/22/2012 07:21 PM, Omair Majid wrote:
> 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.
Yippee!-)
>
>> > 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?
I have no objections here. I guess any participant should be here (eg translators, correctors, testers...)
However -behind my guess is no knowledge;)
>
>> > * 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.
Yes this must be outside jar. It s the image used in "java -splash=image"
Whole the work inside are pure vectors.
I do not see any "sane" access from bash launcher script into jar. And bash launcher script the only place where this image is used.
During your reviews I have noticed that you are not fan of "double splash" - the bitmap for java which will then swap to custom or internal vector one.
Can I ask why? I think that can be useful to show that icedtea-web is launching. Eg for slowly downloaded jnlps or on systems where java needs to !page-fault some processes before its own launch it is quite essential.
>
>> > *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.
sure
>
>> > * 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.
Not true - see comment little bit lower.
>
>> > ;;
>> > 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?
Exactly! the java-spalsh-image precede an vector netx's one. And image and "empty" (no watter in text) vectro one are identical. So swap is quite fluent.
>
>> > 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?
Wou! Interesting idea. I will dig around in next review round.
>
>> > /* 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 :(
hmm So your idea is to remove whole synchronized block, invoke and wait and replace it with:
voaltile splashScreen;
....
splashScreen.setSplashImageURL(splashImageURL);
plashScreen = new JNLPSplashScreen(resourceTracker, file);
?
At least we will get rid of while (splashScreen == null); :)
>
>> > + } 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.
>
True. This was changed this way so EDT shoud be used.
>> > }
>> > -
>> > +
>> > 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 ?
This is testing class. But as you wish:)
>
>> > + 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?
I don't think so. This just javaws spalsh, so each application have its own JNLPSplashScreen. And caching itself is handled by resourceTracker, didn't it?
This will not work (as it is now). Until the image is downloaded the java -splash is shown. Then it swap directly to custom one. However it is interesting idea. But I'm afraid it will look nasty to show "our" animated splash and then replace it with custom one.
>
>> > + 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?
Yah! The splash of javaws and applet ale little bit different (mostly by number of shown elements).
But yes. From my current view is should be enough when "information" element will be set null,
and then it will not be shown (as it does not exists in applet anyway)
I will dig around it for next round.
>
>> > + if (splash != null) {
>> > + splash.spin();
> Can we rename this to something more generic? A quick but bad example
> would be start()?
Definitely :) When I commit, I had some toughts you will not like this name.
will be startAnimation() and stopAnimation() ok?
or better setAnimated(boolean state)?
>
>> > + splash.setInformationContent(InformationElement.createFromJNLP(file));
> Can we use the InformationDesc from the jnlp file directly.
Yes. My class is just wrapper around. It is handling i18n and selecting the best choice for
description (and handling corner cases,and make dealing wit information easier for splash).
>
>> > + this.add(splash.getPanel());
> SplashPanel is a component that can be added to another swing component?
> This I like!
At least something :)
>
>> > + 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?
hmmm. The javadoc is silent about concrete monitor used. Anyway - the splash splitted half on one monitor, half on second is really nasty.
Also when i was testing it, it doesn't seem to behave as you are implying.
>
>> > }
>> >
>> > @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.
sure.
containsCustomImage() / composedOfInternalSpalshScreen()
Can be?
>
>> > +
>> > + 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.
I'm afraid I did not get it. Can you tell little bit abut this proposal?
>
>> > @@ -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?
As told elsewhere, more applets on page *looks* like "race" for splashscreen. this *looks* like helping with issue. But I'm probably very wrong.
For javaws you have mentioned that spalshscreen is for shared runtime. Can it be also this case? ( I don't think so)
But jvm IS shared.... hmmm Im'm confused here.
Also you are mentioning it little bit lower:
>
>
>> > 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?
It IS XEmbeddedFrame :) But yes. Both this and the reflection can be handled by set of interfaces.
>
>> > 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.
I will try to focus to this for enxt round.
>
>> > @@ -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?
Not. it ws moved to dfferent parts of class. It should be deleted.
>
>> > 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?
>
sure.
>
>> > +
>> > + private class PluginAppletPanel extends JPanel {
>> > + // Nothing to do . Yet..
>> > + }
>> >
> What's the purpose of this class?
Sorry. It Have been forgotten :)
>
> Cheers,
> Omair
TYVM for deep-going review (as alwys ;)!
J.
More information about the distro-pkg-dev
mailing list