Icedtea-web splashscreen implementation

[Jiri Vanek]

On 10/23/2012 07:51 PM, Jiri Vanek wrote:
> 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.

Between this patch and the last one, I have noticed regression, that applets splash have started to blink ( really a lot, it is actually useless now). Although the changes in drawing are really minor! (eg update->paint, removed @Override update...)
I will elaborate a bit on this. If you will find time during review, do you mind to verify? (maybe because of my machine can be tired because of late hour?:o)

J.