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

[Alexey Ivanov]

On Fri, 14 Apr 2023 22:48:41 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 one additional commit since the last revision:
> 
>   code cleanup and test changes as requested in code review

Changes requested by aivanov (Reviewer).

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

> 1: /*
> 2:  * Copyright (c) 2018, 2023 Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 1995, 2023, Oracle and/or its affiliates. All rights reserved.

You always update the second year leaving the first one intact. The comma after the year should remain.

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

> 111:         int missinginfo = flags & ~availinfo;
> 112:         if ((availinfo & (ImageObserver.ERROR | ImageObserver.FRAMEBITS)) == 0
> 113:              && missinginfo != 0) {

Suggestion:

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

Nit: the wrapped line should be aligned to the opening parenthesis of the `if`, otherwise it looks as if the wrapped condition is part of the condition in the parentheses but it is not.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 1:

> 1: /*

I [still](https://github.com/openjdk/jdk/pull/13414#discussion_r1164573562) think the test should not be in `test/jdk/javax/imageio/plugins/gif` because it does **not** test the GIF Image IO plugin. A better location is under `test/java/awt/Clipboard`.

Now that there's no external GIF image file, the bugid directory has become unnecessary. We're striving to keep test structure flatter without creating a new folder for each test as it used to be done previously.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 80:

> 78:             (byte) 0x8f, (byte) 0x19, (byte) 0x05, (byte) 0x00, (byte) 0x3b
> 79:     };
> 80:     private void createGUI() {

Please add a blank line between methods, fields and methods, between class declarations.

A blank line between unrelated fields is also a good idea.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 95:

> 93:         Clipboard sys = Toolkit.getDefaultToolkit().getSystemClipboard();
> 94:         sys.setContents(new MyTransferable(img), null);
> 95: 

The blank line in the end of the method is redundant.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 122:

> 120:         }
> 121: 
> 122:     }

Suggestion:

        if (!latch.await(TIMEOUT, MILLISECONDS)) {
            throw new RuntimeException("Image copying taking too long.");
        } else {
            EventQueue.invokeAndWait(CopyAnimatedGIFTest::dispose);
        }
    }

Let's follow Java Coding Style conventions.

I can't help [raising the question](https://github.com/openjdk/jdk/pull/13414#discussion_r1164559765) again:
> I think it also deserves a comment why the frame is disposed of only when the test doesn't fail. Perhaps, we can drop disposing altogether and let jtreg do the cleanup.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 129:

> 127:         }
> 128:     }
> 129:     private static class imgCanvas extends Canvas {

Class names start with a capital letter, method names and fields start with a lower-case letter.

> Is `ImageCanvas` a better name?

Other points from [this comment](https://github.com/openjdk/jdk/pull/13414#discussion_r1164566145) have been addressed.

test/jdk/javax/imageio/plugins/gif/6176679/CopyAnimatedGIFTest.java line 168:

> 166:     }
> 167: 
> 168: }

Please add a final blank line.

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

PR Review: https://git.openjdk.org/jdk/pull/13414#pullrequestreview-1392427528
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171565235
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171570420
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171586456
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171571310
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171571851
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171577529
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171579809
PR Review Comment: https://git.openjdk.org/jdk/pull/13414#discussion_r1171580741