On Thu, 2 Feb 2023 16:44:11 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
>> Windows implementation of GlassClipboard.cpp uses IStorage interface in
>> `PopImage()` to allocate a temporary file, which is then used to capture
>> Image data from system clipboard and move it to JVM side. In order for
>> temporary file to be removed automatically on `IStorage::Release()`,
>> `StgCreateDocfile` API requires passing STGM_DELETEONRELEASE flag -
>> otherwise the file will be left behind when IStorage is released. Contents
>> of temporary file are immediately copied to a JNI Byte Array after acquiring
>> them from clibpoard, so it is safe to provide this flag and remove the file
>> after `PopImage()` is done.
>>
>> Creating a unit test for this case would a bit difficult, as IStorage and
>> its file are created with random temporary name each time, which we cannot
>> easily access. On the other hand, just scouting the temp directory for any
>> leftover .TMP files might prove false results, as other apps or even JFX
>> itself might use IStorage temporary files for some other purposes than
>> managing clipboard images. As such, because this is a simple API change, I
>> did not make a test for this.
>>
>> Tested this change on Windows 10 and it doesn't break any existing tests.
>> Calling `Clipboard.getSystemClipboard().getImage()` with an image stored
>> inside the system clipboard now leaves no leftover files.
>
> Lukasz Kostyra has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - ClipboardExtImageTest: Add missing copyright notice
> - Add manual test to check for leftover Clipboard files
On Windows - the test fails without the fix and passes with it.
On Mac - (as expected) the test passes with or without fix.
I have a minor comment on the test.
Also, the PR should contain only 2 files -
1. Modified file -
modules/javafx.graphics/src/main/native-glass/win/GlassClipboard.cpp
2. Newly added test file - tests/manual/clipboard/ClipboardExtImageTest.java
Please remove all other files. They are not needed to be checked in.
tests/manual/clipboard/ClipboardExtImageTest.java line 193:
> 191: public void start(Stage primaryStage) throws Exception {
> 192: try {
> 193: /*if (!isWindows()) {
This test can be only Windows specific.
I would enable this check and get rid of `isWindows()` check everywhere.
-------------
Changes requested by aghaisas (Reviewer).
PR: https://git.openjdk.org/jfx/pull/994
