Icedtea-web splashscreen implementation

[Jiri Vanek]

On 10/15/2012 07:39 PM, Omair Majid wrote:
> Hi Jiri,
>
> Sorry for the long delay in reviewing this.

Np:)
Thank you!

>
> 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.

I have checked, and it is simple Object lock. As far as I have not touched this code i kept it as it was.
>
>> +            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.

You are right! It does no sense at all. I have overlooked. As I'm loading image in this step, I have added loop to wait to until image is loaded. Thank you for catch this!
>
>> +        // 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
>

Fixed. Although I have added a little bit longer  comment.
>> +        setLocationRelativeTo(null);
>>       }
>
> Also, is it possible to center to monitor where the application was
> started (as opposed to the primary monitor)?
I mus admit I don't know how:(
At the end i have just desperately googled whole sentences..:-/shame on me/-:
  Instead of primary monitor I can show on active (or random :) )monitor. But it is quite a lot of code to do something I'm not sure is good idea.
So i have left untouched for now:(

>
>
>> +        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.

I have extracted to already widely used launchError method.
>
   snip...
>
>> 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!

No volunteer :(

>
>
>>           appletFrame.appletEventListener = new AppletEventListener(appletFrame, appletFrame);

...

typos fixed

...
>>
>> +                public void run() {
>> +                    splashPanel.getSplashComponent().setVisible(false);
>> +                    splashPanel.stopAnimation();
>
> stopAnimation is called twice here too.
>

Well it is :) I wanted probably to be double sure :)
Count of calls reduced to one for each method (I was hesitating which one is better. At the end I kept stopAnimation in run{} block)
>
>>        * 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?

Fixed

>
> But perhaps a better approach would be to only override paintComponent()
> method in this class, and not paint()/update().

Well probably. But I'm hesitating to touch this class more then I'm already touching.

>

Thank you for comments!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: splash_Integration_VIII.diff
Type: text/x-patch
Size: 32647 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121023/0f06cec5/splash_Integration_VIII.diff