[icedtea-web] RFC: Patch to double-buffer applets

[Omair Majid]

On 11/04/2010 02:14 PM, Deepak Bhole wrote:
> * Omair Majid<omajid at redhat.com>  [2010-11-03 09:32]:
>> Hi Deepak,
>>
>> On 11/02/2010 05:37 PM, Deepak Bhole wrote:
>>> * Deepak Bhole<dbhole at redhat.com>   [2010-11-02 17:31]:
>>>> Hi,
>>>>
>>>> IcedTea plugin currently does not double-buffer the applet panel. As a
>>>> result, some applets may flicker depending on the machine.
>>>>
>>>> Attached patch buffers the frame offscreen and then draws it, thereby
>>>> addressing the issue.
>>>>
>>
>> Is there a specific test case that this fixes? The only flickering
>> applet that I know of is at
>> http://download.oracle.com/javase/1.4.2/docs/guide/misc/applet.html,
>> and I dont see any change in that applet with the patch applied.
>>
>>>> ChangeLog:
>>>> 2010-11-02  Deepak Bhole<dbhole at redhat.com>
>>>>
>>>>     * plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (update):
>>>>     Override method and implement double-buffering.
>>>>
>>>> Cheers,
>>>> Deepak
>>>
>>>
>>> Doh! There shouldn't be a println in there. New patch attached. I also
>>> fixed the variable names for the image and its graphics.
>>>
>>>
>>>> diff -r 6c2527d42900 ChangeLog
>>>> --- a/ChangeLog	Mon Nov 01 11:44:15 2010 -0400
>>>> +++ b/ChangeLog	Tue Nov 02 17:29:03 2010 -0700
>>>> @@ -1,3 +1,8 @@
>>>> +2010-11-02  Deepak Bhole<dbhole at redhat.com>
>>>> +
>>>> +	* plugin/icedteanp/java/sun/applet/PluginAppletViewer.java (update):
>>>> +	Override method and implement double-buffering.
>>>> +
>>>>   2010-11-01  Omair Majid<omajid at redhat.com>
>>>>
>>>>   	* Makefile.am (clean-IcedTeaPlugin): Only delete launcher directory if it
>>>> diff -r 6c2527d42900 NEWS
>>>> --- a/NEWS	Mon Nov 01 11:44:15 2010 -0400
>>>> +++ b/NEWS	Tue Nov 02 17:29:03 2010 -0700
>>>> @@ -12,3 +12,4 @@
>>>>
>>>>   * Initial release of IcedTea-Web
>>>>   * PR542: Plugin fails with NPE on http://www.openprocessing.org/visuals/iframe.php?visualID=2615
>>>> +* Applets are now double-buffered to eliminate flicker in ones that do heavy drawing
>>>> diff -r 6c2527d42900 plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
>>>> --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Mon Nov 01 11:44:15 2010 -0400
>>>> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java	Tue Nov 02 17:29:03 2010 -0700
>>>> @@ -359,6 +366,9 @@
>>>>
>>>>        private static Long requestIdentityCounter = 0L;
>>>>
>>>> +     private Image bufferedPanelImage;
>>>> +     private Graphics bufferedImageGraphics;
>>>> +
>>>>        /**
>>>>         * Null constructor to allow instantiation via newInstance()
>>>>         */
>>>> @@ -2163,4 +2178,24 @@
>>>>           }
>>>>       }
>>>>        }
>>>> +
>>>> +     /**
>>>> +      * {@inheritDoc}
>>>> +      */
>>>> +     public void update(Graphics g) {
>>>> +
>>>> +    	 System.err.println("Buffering and drawing");
>>>> +    	
>>>> +    	 // If the image or the graphics don't exist, create new ones
>>>> +    	 if (bufferedPanelImage == null || bufferedImageGraphics == null) {
>>>> +    		 bufferedPanelImage = createImage(getWidth(), getHeight());
>>>> +    		 bufferedImageGraphics = bufferedPanelImage.getGraphics ();
>>>> +    	 }
>>>> +
>>>> +    	 // Paint off-screen
>>>> +    	 paint(bufferedImageGraphics);
>>
>> The overridden method is update, but paint is called here instead.
>> Is there a reason for that? As far as I know, the semantics of
>> update() are slightly different from paint() - update() clears the
>> background and then calls paint(). Perhaps super.update() might be
>> more appropriate?
>>
>
> super.update() would just call paint on the Graphics instance passed to
> it. Container's (super in this case) update() methods do a couple of
> additional checks, but we don't need them for applets, which are known
> lightweight containers.
>
> I can definitely add the call if you feel it would be cleaner though --
> just that it doesn't make a difference in this case.
>

Thanks for the explanation. Its makes more sense now. Please go ahead 
and commit the patch you posted. If you could add this explanation as a 
comment to the patch too, that would be great.

Cheers,
Omair