RFR: 6176679: Application freezes when copying an animated gif image to the system clipboard [v11]

[Alexey Ivanov]

On Sun, 30 Apr 2023 21:34:53 GMT, Rajat Mahajan <rmahajan at openjdk.org> wrote:

>> **Problem:**
>> 
>> On pressing the Copy button we keep waiting in AWT-EventQueue thread in reconstruct() function as we detect that we have missing information for the animated image since we are copying single frame at a time.
>> Due to this infinite wait the application freezes.
>> 
>> **Proposed Fix:**
>> 
>> There are conditions in the reconstruct() function that avoid entering the wait() code if we have some error reading the image , etc. So, I added the condition to avoid entering the wait() code if we are copying single frame at a time. This sounded logical to me since if we have incomplete information(single frame) about the animated image we shouldn’t keep waiting, as it leads to infinite wait. 
>> After this change I see the GIF image being correctly copied and animated.
>> 
>> 
>> **Testing:**
>> 
>> Added a test for this (bug6176679.java) and tested locally on my Windows machine and on mach5.
>
> Rajat Mahajan has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Update ImageRepresentation.java
>    
>    code cleanup
>  - Update ImageRepresentation.java
>    
>    code cleanup
>  -  update fix to work for both cases where image is displayed and not displayed

Changes requested by aivanov (Reviewer).

src/java.desktop/share/classes/sun/awt/image/ImageRepresentation.java line 1:

> 1: /*

Could you remove the unused imports, please?

src/java.desktop/share/classes/sun/awt/image/ImageRepresentation.java line 119:

> 117:                 while ((availinfo &
> 118:                         (ImageObserver.ERROR | ImageObserver.FRAMEBITS)) == 0 &&
> 119:                        missinginfo != 0)

In my opinion, such formatting looks confusing, keeping both ERROR and FRAMEBITS on the same line improves readability even though it doesn't fit 80-column limit.
Suggestion:

                while ((availinfo & (ImageObserver.ERROR | ImageObserver.FRAMEBITS)) == 0
                       && missinginfo != 0) 


Or alternatively, wrap `FRAMEBITS`:
Suggestion:

                while ((availinfo & (ImageObserver.ERROR
                                     | ImageObserver.FRAMEBITS)) == 0
                       && missinginfo != 0)

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java line 54:

> 52: public class CopyAnimatedGIFTest {
> 53:     private static final long TIMEOUT = 10000;
> 54:     private static final CountDownLatch latch = new CountDownLatch(1);

You use the latch twice but initialise only once — the second test will never fail. You have to create it each time.

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java line 90:

> 88: 
> 89:     private void createGUI() {
> 90:         img = Toolkit.getDefaultToolkit().createImage(imageData);

`getImage` to avoid code duplication?

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java line 106:

> 104:     }
> 105: 
> 106:     private void runTest(boolean isImageDisplayed) throws AWTException, InterruptedException, InvocationTargetException {

The line is too long.
Suggestion:

    private void runTest(boolean isImageDisplayed) throws Exception {

We don't really care which exceptions the test method throws.

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java line 112:

> 110:             EventQueue.invokeAndWait(test::createGUI);
> 111:         }
> 112:         else {

Suggestion:

        } else {

In Java code style, you keep both the closing brace, the `else` keyword and the opening brace on the same line.

test/jdk/java/awt/Clipboard/CopyAnimatedGIFTest.java line 142:

> 140:     private static class ImageCanvas extends Canvas {
> 141:         private final Image img;
> 142:         public ImageCanvas(Image img) {

Suggestion:

        private final Image img;

        public ImageCanvas(Image img) {

-------------

PR Review: https://git.openjdk.org/jdk/pull/13414#pullrequestreview-1409684703
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182918324
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182915886
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182929831
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182925024
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182926710
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182928209
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1182936107