On Sun, 30 Apr 2023 21:34:53 GMT, Rajat Mahajan <rmaha...@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
