Icedtea-web splashscreen implementation

[Jiri Vanek]

On 10/23/2012 08:16 PM, Jiri Vanek wrote:
> 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)
>

Wou. And even one more regression - vector splash for javaws stoped to show itself completely. Damn it :(
> J.