Icedtea-web splashscreen implementation

[Jiri Vanek]

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

Fixed patch. Sorry for noise:(

The blinking was caused by removed @Overriden update method in PluginAppletViewer. I had the reason 
to add it :)  Comment in paint method of the same class is explaining reason.

The disappearance for javaws was caused by  wrong applying of patch (for Launcher and GuiLauncher.


J

-------------- next part --------------
A non-text attachment was scrubbed...
Name: splash_Integration_IX.diff
Type: text/x-patch
Size: 33666 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121024/5483f892/splash_Integration_IX.diff