Icedtea-web splashscreen implementation
Omair Majid
omajid at redhat.com
Mon Oct 15 10:39:20 PDT 2012
Hi Jiri,
Sorry for the long delay in reviewing this.
On 08/13/2012 11:15 AM, Jiri Vanek wrote:
> Attached is patch with integration "adapted for head"
Comments in-line below.
> + synchronized (mutex) {
Please consider renaming 'mutex' to indicate what code it is making
mutually exclusive.
> + 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);
> + } catch (InvocationTargetException ite) {
> + ite.printStackTrace();
> }
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + @Override
> + public void run() {
> + splashScreen.setSplashImageURL(splashImageURL);
> + }
> + });
> + } catch (InterruptedException ie) {
> + // Wait till splash screen is created
> + while (splashScreen == null);
This doesn't make sense to me. The variable would have bee initialized
in the previous try block. If it didn't (say an invocation exception was
thrown in the previous try block and this try block throws an
interrupted exception), than this will cause an infinite loop.
> + // 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
Please remove the commented out code. The comment by itself should be
enough:
// center to middle of primary monitor
> + setLocationRelativeTo(null);
> }
Also, is it possible to center to monitor where the application was
started (as opposed to the primary monitor)?
> + 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);
Please consider letting LaunchHander (or some new interface) handle this
without invoking SplashUtils directly. It just adds extra code
everywhere we want to handle exceptions. It's easy for a programmer to
miss it.
> - protected AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> + protected synchronized AppletInstance createApplet(JNLPFile file, boolean enableCodeBase, Container cont) throws LaunchException {
> + // appletInstance is needed by ServiceManager when looking up
> + // services. This could potentially be done in applet constructor
> + // so initialize appletInstance before creating applet.
I dont know if it's supported. The docs state that methods like
getAppletContext may not work before init() is invoked. I would expect
services to be unavailable too:
http://docs.oracle.com/javase/6/docs/api/java/applet/Applet.html#Applet()
> + AppletInstance appletInstance = null;
> + ThreadGroup group = Thread.currentThread().getThreadGroup();
> try {
> JNLPClassLoader loader = JNLPClassLoader.getInstance(file, updatePolicy);
>
> if (enableCodeBase) {
> loader.enableCodeBase();
> } else if (file.getResources().getJARs().length == 0) {
> - throw new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
> + Exception ex = new ClassNotFoundException("Can't do a codebase look up and there are no jars. Failing sooner rather than later");
> + try {
> + SplashUtils.showError(ex, new AppletEnvironment(file, new AppletInstance(file, group, new ClassLoader() {
> + }, null)));
> + } catch (Exception exx) {
> + if (JNLPRuntime.isDebug()) {
> + exx.printStackTrace();
> + }
> + }
> + throw ex;
> }
>
> - ThreadGroup group = Thread.currentThread().getThreadGroup();
> -
> - // appletInstance is needed by ServiceManager when looking up
> - // services. This could potentially be done in applet constructor
> - // so initialize appletInstance before creating applet.
> - AppletInstance appletInstance;
Ah, you just moved this code to the beginning of the function. Never
mind the comment above.
> diff -r a86af88a8ecd plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon Aug 13 15:52:03 2012 +0200
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java Mon Aug 13 16:52:21 2012 +0200
I am not very familiar with this code. Another set of eyes will be
appreciated!
> appletFrame.appletEventListener = new AppletEventListener(appletFrame, appletFrame);
> panel.addAppletListener(appletFrame.appletEventListener);
> + // Clear bindings, if any
"Bindings" ?
> + if (applets.containsKey(identifier)) {
> + PluginAppletViewer oldFrame = applets.get(identifier);
> + oldFrame.remove(panel);
> + panel.removeAppletListener(oldFrame.appletEventListener);
> + }
> @@ -383,9 +401,107 @@
> };
>
> addWindowListener(windowEventListener);
> + final AppletPanel fPanel = panel;
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + public void run() {
> + add("Center", fPanel);
> + fPanel.setVisible(false);
> +
> + splashPanel = SplashUtils.getSplashScreen(fPanel.getWidth(), fPanel.getHeight());
> + if (splashPanel != null) {
> + splashPanel.startAnimation();
> + PluginDebug.debug("Added splash " + splashPanel);
> + add("Center", splashPanel.getSplashComponent());
> + }
> + pack();
> + }
> + });
> + } catch (Exception e) {
> + e.printStackTrace(); // Not much we can do other and print
s/and/than
> + }
>
> }
>
> + public void replaceSplash(final SplashPanel newSplash) {
> + // Loading done. Remove splash screen.
> + if (splashPanel == null) {
> + return;
> + }
> + if (newSplash == null) {
> + removeSplash();
> + return;
> + }
> + splashPanel.stopAnimation();
stopAnimation is called here.
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + public void run() {
> + splashPanel.getSplashComponent().setVisible(false);
> + splashPanel.stopAnimation();
And here. What gives?
> + remove(splashPanel.getSplashComponent());
> + newSplash.setPercentage(splashPanel.getPercentage());
> + newSplash.setSplashWidth(splashPanel.getSplashWidth());
> + newSplash.setSplashHeight(splashPanel.getSplashHeight());
> + newSplash.adjustForSize();
> + splashPanel = newSplash;
> + add("Center", splashPanel.getSplashComponent());
> + pack();
> + }
> + });
> + } catch (Exception e) {
> + e.printStackTrace(); // Not much we can do other and print
s/and/than
> + }
> + }
> +
> + @Override
> + public void removeSplash() {
> +
> + // Loading done. Remove splash screen.
> + if (splashPanel == null) {
> + return;
> + }
> + splashPanel.stopAnimation();
> + try {
> + SwingUtilities.invokeAndWait(new Runnable() {
> +
> + public void run() {
> + splashPanel.getSplashComponent().setVisible(false);
> + splashPanel.stopAnimation();
stopAnimation is called twice here too.
> * the parent class's update() just does a couple of checks (both of
> * which are accounted for) and then calls paint anyway.
> */
> - public void update(Graphics g) {
> + public void paint(Graphics g) {
>
> // If the image or the graphics don't exist, create new ones
> if (bufFrameImg == null || bufFrameImgGraphics == null) {
> @@ -2112,11 +2248,18 @@
> }
>
> // Paint off-screen
> - paint(bufFrameImgGraphics);
> + for (Component c: this.getComponents()) {
> + c.update(bufFrameImgGraphics);
This should be c.paint(), no?
But perhaps a better approach would be to only override paintComponent()
method in this class, and not paint()/update().
Cheers,
Omair
--
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95 0056 F286 F14F 6648 4681
More information about the distro-pkg-dev
mailing list