Re: RFR: 8327856: Convert applet test SpanishDiacriticsTest.java to a main program
On Tue, 12 Mar 2024 17:05:04 GMT, Phil Race wrote: >> test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java line 60: >> >>> 58: expected behaviour for this test. >>> 59: >>> 60:If the text field displays ''o, (i.e. o should be without an >>> acute) >> >> Suggestion: >> >>If the text field displays \u00f3, (i.e. o should be without an acute) >> >> Use the real character? > > It isn't a single char. It is as written - the sequence of 3 characters - > "single quote" "single quote", "latin lower case o" I misread it. If the text is `''o` instead of `ó`… - PR Review Comment: https://git.openjdk.org/jdk/pull/18208#discussion_r1521839155
Re: RFR: 8320079: The ArabicBox.java test has no control buttons [v2]
On Tue, 12 Mar 2024 16:36:15 GMT, Phil Race wrote: >> Alexey Ivanov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use the correct path to library > > test/jdk/java/awt/font/TextLayout/ArabicBox.java line 38: > >> 36: * @bug 4427483 >> 37: * @summary Arabic text followed by newline should have no missing glyphs >> 38: * @library /open/test/jdk/java/awt/regtesthelpers > > Are you sure about that path prefix ? this works properly on a purely open > repo ? My bad, I forgot to update the path. I might've used a wrong command after copying the file around and didn't notice it. Updated now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18234#discussion_r1521810999
Re: RFR: 8320079: The ArabicBox.java test has no control buttons [v2]
> The test is converted to use `PassFailJFrame`. > > I confirmed the updated test reproduce the original problem described in > [JDK-4427483](https://bugs.openjdk.org/browse/JDK-4427483) where Arabic text > displayed as missing glyphs. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Use the correct path to library - Changes: - all: https://git.openjdk.org/jdk/pull/18234/files - new: https://git.openjdk.org/jdk/pull/18234/files/ef948140..bba0baae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18234&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18234&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18234/head:pull/18234 PR: https://git.openjdk.org/jdk/pull/18234
Re: RFR: 8327972: Convert java/awt/FileDialog/SaveFileNameOverrideTest/SaveFileNameOverrideTest.html applet test to main
On Tue, 12 Mar 2024 12:34:57 GMT, Alexander Zvegintsev wrote: > The test is converted to main. > Tested on Linux, Macos and Windows. Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18231#pullrequestreview-1931511748
Re: RFR: 8327924: Simplify TrayIconScalingTest.java
On Tue, 12 Mar 2024 08:33:41 GMT, Alexey Ivanov wrote: > This is to simplify `TrayIconScalingTest.java`. > > 1. Rename `createAndShowGUI` to `createAndShowTrayIcon` which is more > specific. > 2. Move creating tray icon to the top. > 3. Streamline PassFailJFrame with the chained calls, including > `awaitAndCheck`. > 4. Ensure tray is not null before removing the icon. > 5. Pass AWTException as the cause in the wrapped exception. > > Previously, before the builder pattern was introduced, > `PassFailJFrame.positionTestWindow` had had to be called to show the > instructions UI. It's not required any more. Additionally, it has the effect > of moving the instruction frame to the left, which doesn't look good and > sometimes results in positioning the instructions outside of the screen > bounds if the screen resolution is low. > > With the builder pattern, the call to the `build` method shows all the > registered windows on the screen, including the instruction frame. Since > there's no secondary UI, the instructions remain in the centre of the screen. @honkar-jdk, could you, as the author of the test, take a look, please? - PR Comment: https://git.openjdk.org/jdk/pull/18224#issuecomment-1991753900
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 13:42:28 GMT, Alexey Ivanov wrote: >> Create a new bug to improve and to clean up the test then. > > At the same time, I think you should use the term _“shortcut (.lnk file)”_ in > the instructions so that it's clear. > > The thing is .lnk files are known as shortcuts for most users, even though in > Windows Shell API uses link internally which is reflected in the file > extension. > Create a new bug to improve and to clean up the test then. I see, there's an existing bug—[JDK-8146446](https://bugs.openjdk.org/browse/JDK-8146446)—against this test. Although the bug describes that memory usage somewhat grows while the background thread is running, it actually does. It has just grown for me from 100MB to 300MB. Running `System.gc` reduces the memory consumption to 200 MB. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521509910
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v7]
On Tue, 12 Mar 2024 13:52:28 GMT, Prasanta Sadhukhan wrote: >> Conversion of manual applet test to main based using PassFailJFrame manual >> framework > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > instruction update Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18180#pullrequestreview-1931145301
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 13:39:43 GMT, Alexey Ivanov wrote: >> SInce this exercise is just to convert applet to main, I tried to keep the >> instructions same (although I agree it seems ambiguous and I also am not >> clear what needs to be done) which seems to be ok with the tester since 2009 >> when the test was written. >> I think whatever test change needs to be done should be done in JDK-8146446 >> as till then this test will not be run.. >> >> ELse if you disagree, what you propose to be written in the "instructions"? > > Create a new bug to improve and to clean up the test then. At the same time, I think you should use the term _“shortcut (.lnk file)”_ in the instructions so that it's clear. The thing is .lnk files are known as shortcuts for most users, even though in Windows Shell API uses link internally which is reflected in the file extension. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521489601
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 12:47:55 GMT, Prasanta Sadhukhan wrote: >> It seems a Windows shortcut is crucial to reproducing the original problem, >> if it is, the handler for the Start button could show an error message about >> this fact. >> >> Perhaps, if this link cannot be converted to a file in `MyThread` >> constructor, `FileNotFoundException` should be propagated and caught in the >> Start button actionPerformed handler to show an error message and let the >> tester use another Windows shortcut. > > SInce this exercise is just to convert applet to main, I tried to keep the > instructions same (although I agree it seems ambiguous and I also am not > clear what needs to be done) which seems to be ok with the tester since 2009 > when the test was written. > I think whatever test change needs to be done should be done in JDK-8146446 > as till then this test will not be run.. > > ELse if you disagree, what you propose to be written in the "instructions"? Create a new bug to improve and to clean up the test then. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521485042
RFR: 8320079: The ArabicBox.java test has no control buttons
The test is converted to use `PassFailJFrame`. I confirmed the updated test reproduce the original problem described in [JDK-4427483](https://bugs.openjdk.org/browse/JDK-4427483) where Arabic text displayed as missing glyphs. - Commit messages: - 8320079: The ArabicBox.java test has no control buttons Changes: https://git.openjdk.org/jdk/pull/18234/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18234&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8320079 Stats: 103 lines in 1 file changed: 103 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18234.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18234/head:pull/18234 PR: https://git.openjdk.org/jdk/pull/18234
Re: RFR: 8327856: Convert applet test SpanishDiacriticsTest.java to a main program
On Mon, 11 Mar 2024 23:02:53 GMT, Phil Race wrote: > Converts the manual applet test in the files .. > SpanishDiacriticsTest/SpanishDiacriticsTest.html > SpanishDiacriticsTest/SpanishDiacriticsTest.java > .. to a main program using PassFailJFrame > > Added requires windows since it seems very windows-specific. > > I got rid of the redundant extra directory which makes the .java file "new" > according to github but it is largely changed anyway test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java line 60: > 58: expected behaviour for this test. > 59: > 60:If the text field displays ''o, (i.e. o should be without an acute) Suggestion: If the text field displays \u00f3, (i.e. o should be without an acute) Use the real character? test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java line 71: > 69: .rows(18) > 70: .columns(50) > 71: .testUI(() -> createTestUI()) Suggestion: .testUI(SpanishDiacriticsTest::createTestUI) I prefer method references in such cases, it looks cleaner. test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java line 72: > 70: .columns(50) > 71: .testUI(() -> createTestUI()) > 72: .testTimeOut(5) Default timeout could be omitted. test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java line 89: > 87: frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE); > 88: frame.pack(); > 89: frame.setVisible(true); Suggestion: frame.pack(); Calling `setVisible(true)` here causes flickering as the frame gets shown at its default location and later gets moved to its final location. Setting close operation is redundant, `PassFailJFrame` handles closing test UI (when you use the builder pattern) and fails the test. - PR Review Comment: https://git.openjdk.org/jdk/pull/18208#discussion_r1521405668 PR Review Comment: https://git.openjdk.org/jdk/pull/18208#discussion_r1521410458 PR Review Comment: https://git.openjdk.org/jdk/pull/18208#discussion_r1521410885 PR Review Comment: https://git.openjdk.org/jdk/pull/18208#discussion_r1521409226
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 12:37:18 GMT, Alexey Ivanov wrote: >> test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 60: >> >>> 58: >>> 59: 1. Create a link >>> 60: 2. Copy path to the link into TextField >> >> I understand the instructions aren't new but I can't comprehend what it's >> required. >> >> Does it mean a Windows shortcut, `.lnk` file? Seems like it's the case >> because `getLinkLocation` is called which resolves a shortcut to its >> original file. >> >> Can the text field be pre-populated with a default? There could be shortcuts >> on Desktop, there are many shortcuts in the Start menu. > > Could you amend the instructions to clarify them? > > Define what is a _link_. It seems a Windows shortcut is crucial to reproducing the original problem, if it is, the handler for the Start button could show an error message about this fact. Perhaps, if this link cannot be converted to a file in `MyThread` constructor, `FileNotFoundException` should be propagated and caught in the Start button actionPerformed handler to show an error message and let the tester use another Windows shortcut. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521398715
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 10:59:48 GMT, Alexey Ivanov wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Review comment update > > test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 60: > >> 58: >> 59: 1. Create a link >> 60: 2. Copy path to the link into TextField > > I understand the instructions aren't new but I can't comprehend what it's > required. > > Does it mean a Windows shortcut, `.lnk` file? Seems like it's the case > because `getLinkLocation` is called which resolves a shortcut to its original > file. > > Can the text field be pre-populated with a default? There could be shortcuts > on Desktop, there are many shortcuts in the Start menu. Could you amend the instructions to clarify them? Define what is a _link_. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521390282
Re: RFR: 8316388: Opensource five Swing component related regression tests [v2]
On Tue, 12 Mar 2024 11:58:40 GMT, Alexey Ivanov wrote: >> It is not unexpected but instead of failing i think test should just pass. >> Which will happen because the internal frame is not minimized so the test >> condition is not recreated. > > I believe it's unexpected because we don't expect `PropertyVetoException`, in > which case failure seems a better option. > > If anything changes so that `PropertyVetoException` would be thrown all the > time, the test would appear to be passing whereas it would not run at all. This is the only point with which I *disagree*. - PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521362228
Re: RFR: 8316388: Opensource five Swing component related regression tests [v2]
On Tue, 12 Mar 2024 12:05:33 GMT, Alexey Ivanov wrote: >> This is a very simple case and sleeping worked for it so i do not see reason >> to rewrite it with CDL. > > Using CDL can reduce the execution time… likely the three clicks are handled > more quickly than in 3 seconds. On the other hand, overengineering a simple case could do more harm… as using `CountDownLatch` isn't straightforward in this case. - PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521358716
Re: RFR: 8316388: Opensource five Swing component related regression tests
On Tue, 12 Mar 2024 09:34:38 GMT, Alexander Zuev wrote: > I would prefer to keep the directory structure - it does not make any > difference after al and sometimes makes test logs more readable. At times, I find it easier to manage in IDE either. Just mark one folder as Test Sources, and it brings only one test. A folder with lots of tests often results in compilation errors because of duplicate classes and so on. - PR Comment: https://git.openjdk.org/jdk/pull/18184#issuecomment-1991512231
Re: RFR: 8316388: Opensource five Swing component related regression tests [v2]
On Tue, 12 Mar 2024 09:00:18 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java line 69:
>>
>>> 67: keyTyped = true;
>>> 68: bug4773378.this.notifyAll();
>>> 69: }
>>
>> A `CountDownLatch` would work great in this case.
>>
>> And `keyTyped` is a confusing name for a flag which gets to `true` when the
>> internal frame is activated.
>
> Renamed as frameActivated
I'm still for using `CountDownLatch`, it looks cleaner in my opinion.
>> test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java line 115:
>>
>>> 113: try {
>>> 114: SwingUtilities.invokeAndWait(b::setupGUI);
>>> 115: safeSleep(3000);
>>
>> Instead of sleeping, you can use a `CountDownLatch(3)` which you'll
>> `countDown()` for each mouse click. Here you'll call `await(3,
>> TimeUnit.SECONDS)` and throw a timeout error if `await` returns `false`.
>>
>> Another synchroniser may be used to handle the case where
>> `BadLocationException` is thrown to fail the test right away. Alternatively,
>> `BadLocationException` may be wrapped into `RuntimeException` and re-thrown.
>
> This is a very simple case and sleeping worked for it so i do not see reason
> to rewrite it with CDL.
Using CDL can reduce the execution time… likely the three clicks are handled
more quickly than in 3 seconds.
>> test/jdk/javax/swing/JEditorPane/4694598/FrameContent.html line 1:
>>
>>> 1:
>>
>> Can this file be created by the test on the fly? There's no need for a
>> supporting file.
>>
>> In #18147, Prasanta handled the same situation.
>
> I prefer to keep things simpler and not to create an additional possible
> point of failure. The supporting html file is self-explanatory and, if
> needed, can be changed outside of fixing the test itself.
It will make the test self-contained, which, in my opinion, is preferred.
>> test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 29:
>>
>>> 27: * @summary JEditor pane throws NullPointerException on mouse movement.
>>> 28: * @library ../../regtesthelpers
>>> 29: * @build JRobot
>>
>> The test doesn't seem to use any of the extended functionality provided by
>> JRobot, so the standard `java.awt.Robot` can be used instead.
>
> I do not see any harm in using JRobot, it is a Swing test after all.
No harm but a dependency on a library which isn't used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521350860
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521346223
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521347941
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521348935
Re: RFR: 8316388: Opensource five Swing component related regression tests [v2]
On Tue, 12 Mar 2024 11:07:38 GMT, Alexander Zuev wrote: >> Clean up five more tests. >> >> test/jdk/javax/swing/JDesktopPane/4132993/bug4132993.java >> test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java >> test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java >> test/jdk/javax/swing/JEditorPane/4330998/bug4330998.java >> test/jdk/javax/swing/JEditorPane/4694598/FrameContent.html >> test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Bumped copyright year of the affected tests > Minor changes based on the review comments Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18184#pullrequestreview-1930877050
Re: RFR: 8316388: Opensource five Swing component related regression tests [v2]
On Tue, 12 Mar 2024 08:46:45 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JDesktopPane/4132993/bug4132993.java line 47:
>>
>>> 45: } catch (PropertyVetoException ex) {
>>> 46: ex.printStackTrace();
>>> 47: }
>>
>> Shouldn't the test fail if an unexpected exception is thrown?
>
> It is not unexpected but instead of failing i think test should just pass.
> Which will happen because the internal frame is not minimized so the test
> condition is not recreated.
I believe it's unexpected because we don't expect `PropertyVetoException`, in
which case failure seems a better option.
If anything changes so that `PropertyVetoException` would be thrown all the
time, the test would appear to be passing whereas it would not run at all.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1521337005
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v4]
On Tue, 12 Mar 2024 11:39:46 GMT, Alexey Ivanov wrote: >> I think it's needed to have some files in the directory.. >> This test is problemlisted actually and as per [this >> comment](https://bugs.openjdk.org/browse/JDK-8146446?focusedId=13884125&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13884125) >> it needs to be tested on a directory with some files in it but sometimes, >> in CI system temp folder may have thousand of files so I think we should >> revert back to "java.io.tmpdir" for this test sprint and figure out the fix >> for the tmpdir in JDK-8146446...Any objection? > > One way to handle it is to create a number of files in the current directory, > the `scratch` directory that jtreg provides and then rely on jtreg to remove > it. Or to create a temp directory in the current directory and create files > there. > > We have no control over how many files are in the temporary directory, > however, in most cases it's not empty. > I think we should revert back to "java.io.tmpdir" for this test sprint and > figure out the fix for the tmpdir in JDK-8146446...Any objection? Sounds reasonable. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521314423
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v4]
On Tue, 12 Mar 2024 11:24:37 GMT, Prasanta Sadhukhan
wrote:
>> test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 108:
>>
>>> 106:
>>> 107: try {
>>> 108: folder = ShellFolder.getShellFolder(new File("."));
>>
>> I wonder if it was expected that there are files in the directory. Like yes,
>> so I could be wrong. I assumed files were created but no file is created,
>> instead the files in this directory is listed. The expected result is that
>> each file in the directory creates a new `ShellFolder` instance.
>
> I think it's needed to have some files in the directory..
> This test is problemlisted actually and as per [this
> comment](https://bugs.openjdk.org/browse/JDK-8146446?focusedId=13884125&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13884125)
> it needs to be tested on a directory with some files in it but sometimes, in
> CI system temp folder may have thousand of files so I think we should revert
> back to "java.io.tmpdir" for this test sprint and figure out the fix for the
> tmpdir in JDK-8146446...Any objection?
One way to handle it is to create a number of files in the current directory,
the `scratch` directory that jtreg provides and then rely on jtreg to remove
it. Or to create a temp directory in the current directory and create files
there.
We have no control over how many files are in the temporary directory, however,
in most cases it's not empty.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521313859
Re: RFR: 8327750: Convert javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java applet test to main [v2]
On Tue, 12 Mar 2024 10:37:04 GMT, Prasanta Sadhukhan wrote: >> test/jdk/javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java >> line 54: >> >>> 52: .rows(10) >>> 53: .columns(35) >>> 54: .build(); >> >> Suggestion: >> >> .position(PassFailJFrame.Position.TOP_LEFT_CORNER) >> .build(); >> >> To move the instruction frame into its final position right away since >> `build()` shows the instruction UI. Later the instruction frame is moved >> when you use `PassFailJFrame.positionTestWindow`. > > ok. updated..but it doesn't seem to make any difference.. It does. Now the instruction frame is always in the top left corner. Before this update, it showed in the centre of the screen and in a short while moved to the top left corner. - PR Review Comment: https://git.openjdk.org/jdk/pull/18182#discussion_r1521306491
Re: RFR: 8327835: Convert java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest applet test to main [v3]
On Tue, 12 Mar 2024 10:25:28 GMT, Alexander Zvegintsev wrote: >> The `java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest` test is written >> to test the very specific functionality of `XFileDialogPeer`, so it >> shouldn't be run on platforms other than Linux. >> >> We also need to set `sun.awt.disableGtkFileDialogs` to `true` to be able to >> test `XFileDialogPeer`, because by default it tries to use the Gtk file >> dialog if it is available. >> The Gtk file dialog has a search field hidden under the magnifying glass >> icon, but it has a different syntax. >> >> The Windows system file dialog allows you to use wildcard filtering in the >> "File name" field by hitting the `enter` afterwards, but again, this test >> was written to test our own file dialog implementation, not the system's. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > review comments Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18203#pullrequestreview-1930801282
Re: RFR: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main [v3]
On Tue, 12 Mar 2024 10:22:35 GMT, Alexander Zvegintsev wrote: >> The test is converted to main. >> Tested on Linux, Macos and Windows. > > Alexander Zvegintsev has updated the pull request incrementally with one > additional commit since the last revision: > > review comments Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18205#pullrequestreview-1930798500
Re: RFR: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main [v2]
On Tue, 12 Mar 2024 10:27:45 GMT, Alexander Zvegintsev wrote: >> Now its fine on my screen. But its not in center, but top. Does it differ >> for windows and linux? > > It is not in the center, because in the latest commit I added > `.position(PassFailJFrame.Position.TOP_LEFT_CORNER)`, as Alexey suggested. I have 1920×1200 with 150%. This is why it doesn't fit. The second frame overlapped instructions a bit. - PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1521302367
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v4]
On Tue, 12 Mar 2024 09:47:37 GMT, Tejesh R wrote: >> Convert javax/swing/border/Test4129681.java applet test to main based test >> using PassFailJFrame. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review updates Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18189#pullrequestreview-1930781424
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v3]
On Tue, 12 Mar 2024 09:08:08 GMT, Alexey Ivanov wrote: >> Tejesh R has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review updates > > test/jdk/javax/swing/border/Test4129681.java line 75: > >> 73: main.add(Box.createVerticalStrut(4)); >> 74: main.add(label); >> 75: main.add(Box.createVerticalGlue()); > > A `JPanel` with `BorderLayout` works better here, you can fill the area with > the `JLabel`: > Suggestion: > > JPanel main = new JPanel(new BorderLayout()); > main.setBorder(BorderFactory.createEmptyBorder(8, 8, 8, 8)); > main.add(check, BorderLayout.NORTH); > main.add(label, BorderLayout.CENTER); You didn't change to `JPanel` with `BorderLayout`. Whatever. - PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1521290590
Re: RFR: 8327826: Convert javax/swing/border/Test4243289.java applet test to main [v3]
On Tue, 12 Mar 2024 09:53:43 GMT, Tejesh R wrote: >> Convert javax/swing/border/Test4243289.java applet test to main based test >> using PassFailJFrame. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review updates Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18197#pullrequestreview-1930784580
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v4]
On Tue, 12 Mar 2024 10:54:37 GMT, Prasanta Sadhukhan
wrote:
>> Conversion of manual applet test to main based using PassFailJFrame manual
>> framework
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> move frame init in createUI
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 80:
> 78: .title("JFileChooser Instructions")
> 79: .instructions(INSTRUCTIONS)
> 80: .rows(10)
Suggestion:
.instructions(INSTRUCTIONS)
.testTimeOut(10)
.rows(10)
This test will benefit from a longer time out.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521282483
Re: RFR: 8327751: Convert javax/swing/JInternalFrame/6726866/bug6726866.java applet test to main [v2]
On Tue, 12 Mar 2024 02:51:48 GMT, Prasanta Sadhukhan wrote: >> Conversion of manual applet test to main based using PassFailJFrame manual >> framework > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - Review comment update > - Review comment update Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18186#pullrequestreview-1930764110
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 10:51:47 GMT, Prasanta Sadhukhan wrote: > As I understood, testUI calls the passed method in EDT so I think it's better > to init the swing variables there.. Yes, that's true, if you pass a lambda expression to `testUI`, it is called on EDT. Since `initialize` is called from `createUI`, both methods will be called on EDT. - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521271334
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v4]
On Tue, 12 Mar 2024 10:54:37 GMT, Prasanta Sadhukhan
wrote:
>> Conversion of manual applet test to main based using PassFailJFrame manual
>> framework
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> move frame init in createUI
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 108:
> 106:
> 107: try {
> 108: folder = ShellFolder.getShellFolder(new File("."));
I wonder if it was expected that there are files in the directory. Like yes, so
I could be wrong. I assumed files were created but no file is created, instead
the files in this directory is listed. The expected result is that each file in
the directory creates a new `ShellFolder` instance.
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 177:
> 175: private static void fail(String msg) {
> 176: PassFailJFrame.forceFail();
> 177: throw new RuntimeException(msg);
Suggestion:
PassFailJFrame.forceFail(msg);
`forceFail` will throw the exception on the main thread.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521274628
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521275324
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v3]
On Tue, 12 Mar 2024 02:49:25 GMT, Prasanta Sadhukhan wrote: >> Conversion of manual applet test to main based using PassFailJFrame manual >> framework > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Review comment update test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 52: > 50: import javax.swing.JSlider; > 51: import javax.swing.JTextField; > 52: import javax.swing.SwingUtilities; `SwingUtilities` is unused now. test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 60: > 58: > 59: 1. Create a link > 60: 2. Copy path to the link into TextField I understand the instructions aren't new but I can't comprehend what it's required. Does it mean a Windows shortcut, `.lnk` file? Seems like it's the case because `getLinkLocation` is called which resolves a shortcut to its original file. Can the text field be pre-populated with a default? There could be shortcuts on Desktop, there are many shortcuts in the Start menu. test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 65: > 63: 5. Wait several minutes and observe in the Windows Task > Manager > 64:that Memory Usage of java process is not increasing > 65: If memory usage is increasing, click Fail else click Pass . > """; Suggestion: If memory usage is increasing, click Fail else click Pass."""; - PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521264352 PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521262560 PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1521263213
Re: RFR: 8327750: Convert javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java applet test to main [v2]
On Tue, 12 Mar 2024 03:51:52 GMT, Tejesh R wrote: >> Prasanta Sadhukhan has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Review comment update >> - Review comment update > > test/jdk/javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java > line 53: > >> 51: .instructions(INSTRUCTIONS) >> 52: .rows(10) >> 53: .columns(35) > > I guess testUI and awaitAndCheck can be used here only. It may, but it wouldn't work well. Prasanta's approach works better. - PR Review Comment: https://git.openjdk.org/jdk/pull/18182#discussion_r1521188014
Re: RFR: 8327750: Convert javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java applet test to main [v2]
On Tue, 12 Mar 2024 02:47:25 GMT, Prasanta Sadhukhan wrote: >> Conversion of manual applet test to main based using PassFailJFrame manual >> framework > > Prasanta Sadhukhan has updated the pull request incrementally with two > additional commits since the last revision: > > - Review comment update > - Review comment update Marked as reviewed by aivanov (Reviewer). test/jdk/javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java line 54: > 52: .rows(10) > 53: .columns(35) > 54: .build(); Suggestion: .position(PassFailJFrame.Position.TOP_LEFT_CORNER) .build(); To move the instruction frame into its final position right away since `build()` shows the instruction UI. Later the instruction frame is moved when you use `PassFailJFrame.positionTestWindow`. - PR Review: https://git.openjdk.org/jdk/pull/18182#pullrequestreview-1930616215 PR Review Comment: https://git.openjdk.org/jdk/pull/18182#discussion_r1521190975
Re: RFR: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main [v2]
On Mon, 11 Mar 2024 23:28:30 GMT, Alexander Zvegintsev
wrote:
>> The test is converted to main.
>> Tested on Linux, Macos and Windows.
>
> Alexander Zvegintsev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review comments
Looks good to me.
Yet I left a couple of suggestions.
test/jdk/java/awt/FileDialog/MultipleMode.java line 74:
> 72: sysout.setEditable(false);
> 73:
> 74: final Checkbox mode = new Checkbox("multiple", true);
Does it make sense to leave the checkbox unselected, provided the instructions
start with unselect the _Multiple_ checkbox (turn off)?
test/jdk/java/awt/FileDialog/MultipleMode.java line 90:
> 88: for (File f : d.getFiles()) {
> 89: sysout.append(" %s\n".formatted(f));
> 90: }
If you like you can replace the for-loop with functional-style stream:
Suggestion:
sysout.append("FILES:\n");
sysout.append(Arrays.stream(d.getFiles())
.map(" %s\n"::formatted)
.collect(Collectors.joining()));
-
Marked as reviewed by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18205#pullrequestreview-1930590396
PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1521173947
PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1521172060
Re: RFR: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main [v2]
On Tue, 12 Mar 2024 05:17:31 GMT, Tejesh R wrote: >> Alexander Zvegintsev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review comments > > test/jdk/java/awt/FileDialog/MultipleMode.java line 101: > >> 99: frame.add(sysout, BorderLayout.CENTER); >> 100: >> 101: frame.pack(); > > Frame size is too big and going out of the screen, might fix a size here. It doesn't fit on the screen of my VM either. One way to resolve this problem is to add .position(PassFailJFrame.Position.TOP_LEFT_CORNER) to `PassFailJFrame` builder as there's no easy way to move the instruction frame to the left so that both frames fit on the screen. - PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1521162323
Re: RFR: 8327835: Convert java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest applet test to main [v2]
On Mon, 11 Mar 2024 23:18:28 GMT, Alexander Zvegintsev
wrote:
>> The `java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest` test is written
>> to test the very specific functionality of `XFileDialogPeer`, so it
>> shouldn't be run on platforms other than Linux.
>>
>> We also need to set `sun.awt.disableGtkFileDialogs` to `true` to be able to
>> test `XFileDialogPeer`, because by default it tries to use the Gtk file
>> dialog if it is available.
>> The Gtk file dialog has a search field hidden under the magnifying glass
>> icon, but it has a different syntax.
>>
>> The Windows system file dialog allows you to use wildcard filtering in the
>> "File name" field by hitting the `enter` afterwards, but again, this test
>> was written to test our own file dialog implementation, not the system's.
>
> Alexander Zvegintsev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> better chaining
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/FileDialog/RegexpFilterTest.java line 65:
> 63: JButton show = new JButton("show");
> 64: show.addActionListener(e ->
> 65: new FileDialog(new Frame()).setVisible(true));
Suggestion:
new FileDialog((Frame) null).setVisible(true));
There's no meaningful owner, `null` should do. Alternatively, you can find the
top level owner of the `show` button, but I don't think it's worth the effort.
-
PR Review: https://git.openjdk.org/jdk/pull/18203#pullrequestreview-1930557050
PR Review Comment: https://git.openjdk.org/jdk/pull/18203#discussion_r1521150843
Re: RFR: 8327835: Convert java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest applet test to main [v2]
On Mon, 11 Mar 2024 23:18:28 GMT, Alexander Zvegintsev
wrote:
>> The `java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest` test is written
>> to test the very specific functionality of `XFileDialogPeer`, so it
>> shouldn't be run on platforms other than Linux.
>>
>> We also need to set `sun.awt.disableGtkFileDialogs` to `true` to be able to
>> test `XFileDialogPeer`, because by default it tries to use the Gtk file
>> dialog if it is available.
>> The Gtk file dialog has a search field hidden under the magnifying glass
>> icon, but it has a different syntax.
>>
>> The Windows system file dialog allows you to use wildcard filtering in the
>> "File name" field by hitting the `enter` afterwards, but again, this test
>> was written to test our own file dialog implementation, not the system's.
>
> Alexander Zvegintsev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> better chaining
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/FileDialog/RegexpFilterTest.java line 59:
> 57:
> 58: public static void main(String[] args) throws Exception {
> 59: new PassFailJFrame.Builder()
Suggestion:
PassFailJFrame.builder()
Please, use the helper method `builder()` instead of instantiating the
`Builder` class explicitly.
-
PR Review: https://git.openjdk.org/jdk/pull/18203#pullrequestreview-1930546191
PR Review Comment: https://git.openjdk.org/jdk/pull/18203#discussion_r1521143484
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v3]
On Tue, 12 Mar 2024 03:29:26 GMT, Tejesh R wrote:
>> Convert javax/swing/border/Test4129681.java applet test to main based test
>> using PassFailJFrame.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review updates
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/border/Test4129681.java line 61:
> 59:
> 60: public static JComponent init() {
> 61: JCheckBox check = new JCheckBox("Enable/Disable");
Suggestion:
JLable label = new JLabel("message");
JCheckBox check = new JCheckBox("Enable/Disable");
And then both are local variables which you can use in lambda expressions.
test/jdk/javax/swing/border/Test4129681.java line 75:
> 73: main.add(Box.createVerticalStrut(4));
> 74: main.add(label);
> 75: main.add(Box.createVerticalGlue());
A `JPanel` with `BorderLayout` works better here, you can fill the area with
the `JLabel`:
Suggestion:
JPanel main = new JPanel(new BorderLayout());
main.setBorder(BorderFactory.createEmptyBorder(8, 8, 8, 8));
main.add(check, BorderLayout.NORTH);
main.add(label, BorderLayout.CENTER);
-
PR Review: https://git.openjdk.org/jdk/pull/18189#pullrequestreview-1930479454
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1521101975
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1521099890
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v2]
On Tue, 12 Mar 2024 03:18:15 GMT, Tejesh R wrote:
>> test/jdk/javax/swing/border/Test4129681.java line 41:
>>
>>> 39:
>>> 40: public class Test4129681 {
>>> 41: private static JLabel label;
>>
>> The `label` does not need to be global, it can be confined to `init`.
>
> Its been used inside `itemListener`.
I guess it was needed but now it's not; so it's better to clean this up.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1521088159
Re: RFR: 8327826: Convert javax/swing/border/Test4243289.java applet test to main [v2]
On Tue, 12 Mar 2024 03:48:37 GMT, Tejesh R wrote:
>> Convert javax/swing/border/Test4243289.java applet test to main based test
>> using PassFailJFrame.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review updates - splitUI
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/border/Test4243289.java line 44:
> 42: public static void main(String[] args) throws Exception {
> 43: String testInstructions = """
> 44: If TiltedBorder with title "Panel Title" is overstriken
> with
Suggestion:
If TitledBorder with title "Panel Title" is overstruck with
Typo in the border class name; _“overstriken”_ is a rare form, more standard
form is _“overstruck”_, see Wiktionary [entry for
“overstrike”](https://en.wiktionary.org/wiki/overstrike#Verb), other
dictionaries agree.
test/jdk/javax/swing/border/Test4243289.java line 59:
> 57:
> 58: public static JComponent init() {
> 59: Font font = new Font("Dialog", Font.PLAIN, 12); // NON-NLS: the
> font name
Suggestion:
Font font = new Font(Font.DIALOG, Font.PLAIN, 12);
I suggest using the constant. The test may have been written before this
constant became available.
test/jdk/javax/swing/border/Test4243289.java line 74:
> 72: main.add(Box.createVerticalGlue());
> 73: main.add(panel);
> 74: main.add(Box.createVerticalGlue());
Suggestion:
main.add(panel);
The glue isn't needed here, the panel handles resizing well.
-
PR Review: https://git.openjdk.org/jdk/pull/18197#pullrequestreview-1930429030
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1521066252
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1521068946
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1521067546
RFR: 8327924: Simplify TrayIconScalingTest.java
This is to simplify `TrayIconScalingTest.java`. 1. Rename `createAndShowGUI` to `createAndShowTrayIcon` which is more specific. 2. Move creating tray icon to the top. 3. Streamline PassFailJFrame with the chained calls, including `awaitAndCheck`. 4. Ensure tray is not null before removing the icon. 5. Pass AWTException as the cause in the wrapped exception. Previously, before the builder pattern was introduced, `PassFailJFrame.positionTestWindow` had had to be called to show the instructions UI. It's not required any more. Additionally, it has the effect of moving the instruction frame to the left, which doesn't look good and sometimes results in positioning the instructions outside of the screen bounds if the screen resolution is low. With the builder pattern, the call to the `build` method shows all the registered windows on the screen, including the instruction frame. Since there's no secondary UI, the instructions remain in the centre of the screen. - Depends on: https://git.openjdk.org/jdk/pull/18206 Commit messages: - Pass AWTException as the cause in the wrapped exception - Re-arrange the code in TrayIconScalingTest.java Changes: https://git.openjdk.org/jdk/pull/18224/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18224&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327924 Stats: 27 lines in 1 file changed: 10 ins; 11 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18224.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18224/head:pull/18224 PR: https://git.openjdk.org/jdk/pull/18224
Re: RFR: 8325851: Hide PassFailJFrame.Builder constructor
On Mon, 11 Mar 2024 22:09:09 GMT, Phil Race wrote:
>> The `Builder` class in `PassFailJFrame` is public and has a public
>> constructor. At the same time, a better design would be to hide all the
>> Builder constructors and rely on the `builder()` method which returns an
>> instance of the `Builder` class.
>>
>> This PR makes the constructor of the `Builder` class private so that it's
>> not accessible directly.
>>
>> Use `PassFailJFrame.builder()` to get an instance of the builder and
>> configure parameters of `PassFailJFrame`.
>>
>> I updated all the tests which used the constructor directly by calling `new
>> PassFailJFrame.Builder()`.
>>
>> I converted a few tests to use the `testUI`, which greatly simplifies the
>> test code. This change also removes flickering of the test UI.
>
> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1070:
>
>> 1068: }
>> 1069:
>> 1070: public Builder title(String title) {
>
> So what about this one ? Are too many tests using it ?
What about them? All the methods in `Builder` return `Builder`, that's
perfectly fine.
The goal of this PR is to make impossible to write
new PassFailJFrame.Builder();
which is replaced with
PassFailJFrame.builder();
In the former case, the test explicitly depends on the fact that the builder
pattern implementation is in the `PassFailJFrame.Builder` class.
In the latter case, I can change the `builder()` method to return another
class. As long as that other class has the same set of methods, like
`instructions` and title`, which return the same object, nothing breaks.
Encapsulation.
With this change, all the existing tests continue to run. Those tests which
failed to compile because of a breaking API change are updated in this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18206#discussion_r1520497593
Re: RFR: 8327826: Convert javax/swing/border/Test4243289.java applet test to main
On Mon, 11 Mar 2024 14:46:11 GMT, Tejesh R wrote:
> Convert javax/swing/border/Test4243289.java applet test to main based test
> using PassFailJFrame.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/border/Test4243289.java line 47:
> 45: When frame starts, you'll see a panel with a TitledBorder
> 46: with title "Panel Title". If this title is overstriken
> with
> 47: the border line, test fails, otherwise it passes.
As in #18189, just explain what the tester needs to do. Both the instructions
and the test UI appear on the screen simultaneously.
test/jdk/javax/swing/border/Test4243289.java line 49:
> 47: the border line, test fails, otherwise it passes.
> 48: """;
> 49: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Suggestion:
PassFailJFrame passFailJFrame = PassFailJFrame.builder()
Please use the `builder` method instead of instantiating the `Builder` object
directly.
test/jdk/javax/swing/border/Test4243289.java line 59:
> 57: SwingUtilities.invokeAndWait(() -> {
> 58: init();
> 59: });
Convert to `testUI` which streamlines the `main` method and simplifies creating
the test UI.
Alternatively, use `splitUI` which could simplify the resulting UI.
test/jdk/javax/swing/border/Test4243289.java line 77:
> 75: frame.setSize(300, 300);
> 76: PassFailJFrame.addTestWindow(frame);
> 77: PassFailJFrame.positionTestWindow(frame,
> PassFailJFrame.Position.TOP_LEFT_CORNER);
Any particular reason why top left corner is used instead of
centered-positioning?
-
PR Review: https://git.openjdk.org/jdk/pull/18197#pullrequestreview-1928952878
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1520332384
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1520317239
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1520318570
PR Review Comment: https://git.openjdk.org/jdk/pull/18197#discussion_r1520320069
Re: RFR: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main
On Mon, 11 Mar 2024 17:53:48 GMT, Alexander Zvegintsev
wrote:
> The test is converted to main.
> Tested on Linux, Macos and Windows.
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/FileDialog/MultipleMode.java line 68:
> 66: .build();
> 67:
> 68: EventQueue.invokeAndWait(MultipleMode::init);
I suggest using `testUI` method. Make your `init` method return `Frame` after
you call `pack()`, don't call `setVisible(true)`.
.testUI(MultipleMode::init)
Then you'll be able to chain `awaitAndCheck()` call.
test/jdk/java/awt/FileDialog/MultipleMode.java line 82:
> 80: Button open = new Button("open");
> 81: open.addActionListener(e -> {
> 82: FileDialog d = new FileDialog((Frame)null);
Make `frame` the owner?
test/jdk/java/awt/FileDialog/MultipleMode/MultipleMode.java line 1:
> 1: /*
Do we move test files when modifying?
-
PR Review: https://git.openjdk.org/jdk/pull/18205#pullrequestreview-1929367696
PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1520452456
PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1520453034
PR Review Comment: https://git.openjdk.org/jdk/pull/18205#discussion_r1520453449
Re: RFR: 8327835: Convert java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest applet test to main
On Mon, 11 Mar 2024 17:29:50 GMT, Alexander Zvegintsev wrote: > The `java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest` test is written > to test the very specific functionality of `XFileDialogPeer`, so it shouldn't > be run on platforms other than Linux. > > We also need to set `sun.awt.disableGtkFileDialogs` to `true` to be able to > test `XFileDialogPeer`, because by default it tries to use the Gtk file > dialog if it is available. > The Gtk file dialog has a search field hidden under the magnifying glass > icon, but it has a different syntax. > > The Windows system file dialog allows you to use wildcard filtering in the > "File name" field by hitting the `enter` afterwards, but again, this test was > written to test our own file dialog implementation, not the system's. Changes requested by aivanov (Reviewer). test/jdk/ProblemList.txt line 799: > 797: java/awt/TrayIcon/DragEventSource/DragEventSource.java 8252242 macosx-all > 798: java/awt/FileDialog/DefaultFocusOwner/DefaultFocusOwner.java 7187728 > macosx-all,linux-all > 799: java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest.html 7187728 > macosx-all,linux-all What about [JDK-7187728](https://bugs.openjdk.org/browse/JDK-7187728)? It's still unresolved. test/jdk/java/awt/FileDialog/RegexpFilterTest.java line 73: > 71: .build(); > 72: > 73: passFailJFrame.awaitAndCheck(); Suggestion: .build() .awaitAndCheck(); Chain it. Then the variable `passFailJFrame` becomes redundant. test/jdk/java/awt/FileDialog/RegexpFilterTest/RegexpFilterTest.java line 1: > 1: /* I believe we haven't flattened the directory structure when modifying tests. On the other hand, this file has no meaningful modification history, so nothing's lost. - PR Review: https://git.openjdk.org/jdk/pull/18203#pullrequestreview-1929348959 PR Review Comment: https://git.openjdk.org/jdk/pull/18203#discussion_r1520449595 PR Review Comment: https://git.openjdk.org/jdk/pull/18203#discussion_r1520446796 PR Review Comment: https://git.openjdk.org/jdk/pull/18203#discussion_r1520448130
RFR: 8325851: Hide PassFailJFrame.Builder constructor
The `Builder` class in `PassFailJFrame` is public and has a public constructor.
At the same time, a better design would be to hide all the Builder constructors
and rely on the `builder()` method which returns an instance of the `Builder`
class.
This PR makes the constructor of the `Builder` class private so that it's not
accessible directly.
Use `PassFailJFrame.builder()` to get an instance of the builder and configure
parameters of `PassFailJFrame`.
I updated all the tests which used the constructor directly by calling `new
PassFailJFrame.Builder()`.
I converted a few tests to use the `testUI`, which greatly simplifies the test
code. This change also removes flickering of the test UI.
-
Commit messages:
- Minor updates to createUI in ComboPopupBug
- Use testUI in ComboPopupBug
- Use testUI in DefaultCloseOperation and reduce the number of columns
- Use testUI in LightweightCliprect
- Remove unused EventQueue import
- Use testUI in DefaultSizeTest
- Use testUI in FrameRepackTest
- Use testUI in MultimonVImage
- Use testUI in AddRemoveMenuBar_{1..4}
- Remove unused EventQueue import
- ... and 5 more: https://git.openjdk.org/jdk/compare/0a6e64e2...91361e52
Changes: https://git.openjdk.org/jdk/pull/18206/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18206&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8325851
Stats: 242 lines in 21 files changed: 14 ins; 118 del; 110 mod
Patch: https://git.openjdk.org/jdk/pull/18206.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18206/head:pull/18206
PR: https://git.openjdk.org/jdk/pull/18206
Re: RFR: 8327137: Add test for ConcurrentModificationException in BasicDirectoryModel
On Mon, 4 Mar 2024 15:52:45 GMT, Alexey Ivanov wrote: > I'm adding a regression test for > [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and > [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091); it's also a > regression test for > [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690). > > I referenced this test in PR #17462 in [this > comment](https://github.com/openjdk/jdk/pull/17462#issuecomment-1914844026). > I fine-tuned the test since that time. > > The test doesn't fail all the time without the fix for JDK-8323670, however, > it fails sometimes. If you run the test several times, it will likely fail > _without the fix_. > > For me, the test fails about 10 times from 40 runs in the CI. It fails on > macOS more frequently than on Linux. > > When the test passes, it usually completes in 5 minutes. > > **How the test works** > > The test creates a temporary directory in the current directory and creates a > number of files in it. (The number of files is controlled by > `NUMBER_OF_THREADS` constant). Then the test creates `JFileChooser` in the > temporary directory. > > The test starts several _scanner_ threads, the number is controlled by > `NUMBER_OF_THREADS`, which repeatedly call > `fileChooser.rescanCurrentDirectory()`. This results in calling > `BasicDirectoryModel.validateFileCache` which starts a background thread — > "Basic L&F File Loading Thread" — to enumerate the files. > > A timer is used to create new files in the directory that the file chooser is > using. > > After enumerating the files, the File Loading Thread posts an event to EDT. > The event updates `fileCache` and fires a `ListDataEvent`. > > If the File Loading Thread is iterating over `fileCache` using `Iterator` > (when `fileCache.subList` or `fileCache.equals` is running; or a new `Vector` > instance is created from a `fileCache` or its sublist) and `fileCache` is > being updated on EDT, then `ConcurrentModificationException` is thrown. > > On Linux and on _headless_ macOS, `ShellFolder.invoke` is executed in the > caller, which makes it easier to reproduce the issue. Because of > [JDK-8325179](https://bugs.openjdk.org/browse/JDK-8325179), there are several > File Loading Threads, which also helps to reproduce the issue. > > On _headful_ macOS, the `BasicDirectoryModel` is not used, so the test does > not reproduce the issue. > > On Windows, the test does not fail or fails with `OutOfMemoryError`. It is > because all the File Loading Threads are serialised on the COM thread, > `ShellFolder.invoke` submits the task to the COM thread and waits for it to > complete. The chance of updating `fileCache` whil... @mrserb, @RealCLanger, could you run this test in your environments, please? It is for Linux and macOS only. I'm looking for confirmation that the test fails without the fix for [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670). Since JDK-8323670 is fixed in mainline, you can run it in 21u or 17u which don't have the fix yet. You'll need to run the test several times because it fails only occasionally. That's why I want to collect more data on whether the test is useful/stable enough. I greatly appreciate your help. - PR Comment: https://git.openjdk.org/jdk/pull/18109#issuecomment-1989244423
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v2]
On Mon, 11 Mar 2024 16:32:38 GMT, Tejesh R wrote: >> Convert javax/swing/border/Test4129681.java applet test to main based test >> using PassFailJFrame. > > Tejesh R has updated the pull request incrementally with three additional > commits since the last revision: > > - Review updates > - Review updates > - Updates > > I propose using testUI method of the builder, it hides the complexity of > > registering and positioning the frame. Modify this method to return the > > created frame. > > If the test UI is simple enough, you can even use splitUI to display the test > UI along with the instructions. In this case, instead of frame return a > JComponent, likely JPanel, which contains all the test UI. It fits into splitUI pretty well, especially if you reduce the size of the test UI frame. Yet you'll have convert `JFrame` to `JPanel` or `Box`. You can find a sample usage in #17847 or #17608 in [Collate2DPrintingTest.java](https://github.com/openjdk/jdk/blob/69fe7d9651cd759eea0495a0b7bf8c356d9b93a6/test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java#L96-L104). - PR Comment: https://git.openjdk.org/jdk/pull/18189#issuecomment-1989179731
Re: RFR: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed." [v2]
On Thu, 7 Mar 2024 20:07:19 GMT, Alisen Chung wrote: >> Introduce delays in test to stabilize. Test passes 50x after fix. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > update copyright, fix jtreg formatting, update frame title, expand imports Looks fine. - Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18091#pullrequestreview-1928660737
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v2]
On Mon, 11 Mar 2024 13:43:56 GMT, Alexey Ivanov wrote:
>> Tejesh R has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Review updates
>> - Review updates
>> - Updates
>
> test/jdk/javax/swing/border/Test4129681.java line 67:
>
>> 65: }
>> 66:
>> 67: public static void init() {
>
> I propose using `testUI` method of the builder, it hides the complexity of
> registering and positioning the frame. Modify this method to return the
> created `frame`.
>
> See PR #17838 as an example or other PRs. The javadoc for `PassFailJFrame`
> also contains the recommended usage.
If the test UI is simple enough, you can even use `splitUI` to display the test
UI along with the instructions. In this case, instead of frame return a
`JComponent`, likely `JPanel`, which contains all the test UI.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519853979
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main [v2]
On Mon, 11 Mar 2024 15:00:21 GMT, Tejesh R wrote:
>> Convert javax/swing/border/Test4129681.java applet test to main based test
>> using PassFailJFrame.
>
> Tejesh R has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - Review updates
> - Review updates
> - Updates
Changes requested by aivanov (Reviewer).
Marked as reviewed by aivanov (Reviewer).
test/jdk/javax/swing/border/Test4129681.java line 41:
> 39:
> 40: public class Test4129681 {
> 41: private static JLabel label;
The `label` does not need to be global, it can be confined to `init`.
test/jdk/javax/swing/border/Test4129681.java line 47:
> 45: public static void main(String[] args) throws Exception {
> 46: String testInstructions = """
> 47: When frame starts, you'll see a checkbox
It's not what I really meant.
The instructions and the frame appear simultaneously, thus the tester doesn't
need to wait for anything. Just describe what the tester needs to do.
test/jdk/javax/swing/border/Test4129681.java line 53:
> 51: is disabled as well as the label.
> 52: """;
> 53: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Suggestion:
PassFailJFrame passFailJFrame = PassFailJFrame.builder()
Use helper method to create the builder, don't use `new` explicitly.
-
PR Review: https://git.openjdk.org/jdk/pull/18189#pullrequestreview-1928044311
PR Review: https://git.openjdk.org/jdk/pull/18189#pullrequestreview-1928078877
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519870582
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519849859
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519855420
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v2]
On Mon, 11 Mar 2024 09:48:36 GMT, Prasanta Sadhukhan
wrote:
>> test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 56:
>>
>>> 54: public class bug6798062 {
>>> 55:
>>> 56: private static final String instructionsText = """
>>
>> `instructionsText` may be capitalize as using capital letter is common
>> convention for final variables.
>
> That is for constant, IIRC...
Either way is fine… And `instructionsText` *is* a constant but it's not public.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1519826585
Re: RFR: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main [v2]
On Mon, 11 Mar 2024 09:53:17 GMT, Prasanta Sadhukhan
wrote:
>> Conversion of manual applet test to main based using PassFailJFrame manual
>> framework
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> typo
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 1:
> 1: /*
Likely, all the catch blocks which simply print stack traces should propagate
the exception or rather in addition to printing the stack trace call
`PassFailJFrame.forceFail` with a corresponding message.
The `fail` method of the test should also call `PassFailJFrame.forceFail` to
fail the test.
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 78:
> 76:
> 77: public static void main(String[] args) throws Exception {
> 78: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Please use the `builder` helper method instead of `new`.
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 86:
> 84: .build();
> 85:
> 86: SwingUtilities.invokeAndWait(() -> {
I suggest using the `testUI` method of the builder which accepts a lambda
expression that returns a frame. Calling the passed method reference or lambda
expression on EDT, registering the frame with `PassFailJFrame` and its
positioning is handled automatically.
test/jdk/javax/swing/JFileChooser/6798062/bug6798062.java line 107:
> 105: }
> 106:
> 107: private JComponent initialize() {
I suggest creating the files in the current directory instead of `temp` or
`home`. When run with jtreg, the current directory is set to `scratch` which is
automatically removed by jtreg, which ensures no files are left behind if the
test fails to clean them up for whatever reason.
-
PR Review: https://git.openjdk.org/jdk/pull/18180#pullrequestreview-1928016162
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1519841469
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1519857497
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1519832650
PR Review Comment: https://git.openjdk.org/jdk/pull/18180#discussion_r1519835478
Re: RFR: 8327750: Convert javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java applet test to main
On Mon, 11 Mar 2024 08:54:38 GMT, Prasanta Sadhukhan
wrote:
> Conversion of manual applet test to main based using PassFailJFrame manual
> framework
test/jdk/javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java
line 60:
> 58: .getInstalledLookAndFeels();
> 59: for (final UIManager.LookAndFeelInfo info : infos) {
> 60: SwingUtilities.invokeAndWait(() -> {
As far as I can see, the test creates multiple frames. This situation isn't
handled well automatically by `PassFailJFrame` so far, no default window layout
is provided.
However, it could not be needed here…
test/jdk/javax/swing/JFileChooser/FileFilterDescription/FileFilterDescription.java
line 70:
> 68: PassFailJFrame.addTestWindow(frame);
> 69: PassFailJFrame.positionTestWindow(frame,
> PassFailJFrame.Position.TOP_LEFT_CORNER);
> 70: frame.pack();
You should call `pack()` before positioning the frame, otherwise the frame
won't be positioned correctly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18182#discussion_r1519817966
PR Review Comment: https://git.openjdk.org/jdk/pull/18182#discussion_r1519815357
Re: RFR: 8327751: Convert javax/swing/JInternalFrame/6726866/bug6726866.java applet test to main
On Mon, 11 Mar 2024 10:02:21 GMT, Prasanta Sadhukhan
wrote:
> Conversion of manual applet test to main based using PassFailJFrame manual
> framework
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/JInternalFrame/6726866/bug6726866.java line 51:
> 49:
> 50: public static void main(String[] args) throws Exception {
> 51: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
Please use the `builder` helper method instead of using `new` explicitly.
test/jdk/javax/swing/JInternalFrame/6726866/bug6726866.java line 54:
> 52: .title("JInternalFrame Instructions")
> 53: .instructions(instructionsText)
> 54: .testTimeOut(5)
The default timeout can be omitted.
-
PR Review: https://git.openjdk.org/jdk/pull/18186#pullrequestreview-1927995890
PR Review Comment: https://git.openjdk.org/jdk/pull/18186#discussion_r1519859043
PR Review Comment: https://git.openjdk.org/jdk/pull/18186#discussion_r1519820170
Re: RFR: 8316388: Opensource five Swing component related regression tests
On Mon, 11 Mar 2024 09:41:47 GMT, Alexander Zuev wrote:
> Clean up five more tests.
>
> test/jdk/javax/swing/JDesktopPane/4132993/bug4132993.java
> test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java
> test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java
> test/jdk/javax/swing/JEditorPane/4330998/bug4330998.java
> test/jdk/javax/swing/JEditorPane/4694598/FrameContent.html
> test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java
The copyright year should be updated to 2024.
The directory structure should be flattened, there's no need for additional
folder in the path.
test/jdk/javax/swing/JDesktopPane/4132993/bug4132993.java line 47:
> 45: } catch (PropertyVetoException ex) {
> 46: ex.printStackTrace();
> 47: }
Shouldn't the test fail if an unexpected exception is thrown?
test/jdk/javax/swing/JDesktopPane/4132993/bug4132993.java line 59:
> 57: throw new RuntimeException("Test interrupted by "
> 58: + e.getLocalizedMessage());
> 59: }
You can add `throws Exception` clause to the `main` method and remove this
`catch` block.
test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java line 69:
> 67: keyTyped = true;
> 68: bug4773378.this.notifyAll();
> 69: }
A `CountDownLatch` would work great in this case.
And `keyTyped` is a confusing name for a flag which gets to `true` when the
internal frame is activated.
test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java line 85:
> 83: public void performTest() {
> 84: try {
> 85: jif.setSelected(true);
This should be called via `SwingUtilities.invokeLater` on EDT.
test/jdk/javax/swing/JDesktopPane/4773378/bug4773378.java line 102:
> 100: } catch (Throwable t) {
> 101: t.printStackTrace();
> 102: }
A `Throwable` should fail the test, it must be propagated.
test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java line 83:
> 81: robo.setAutoDelay(100);
> 82: robo.delay(1000);
> 83: robo.mouseMove(100,100);
This may fail if the frame location of (50, 50) isn't respected by a window
manager.
test/jdk/javax/swing/JEditorPane/4325606/bug4325606.java line 115:
> 113: try {
> 114: SwingUtilities.invokeAndWait(b::setupGUI);
> 115: safeSleep(3000);
Instead of sleeping, you can use a `CountDownLatch(3)` which you'll
`countDown()` for each mouse click. Here you'll call `await(3,
TimeUnit.SECONDS)` and throw a timeout error if `await` returns `false`.
Another synchroniser may be used to handle the case where
`BadLocationException` is thrown to fail the test right away. Alternatively,
`BadLocationException` may be wrapped into `RuntimeException` and re-thrown.
test/jdk/javax/swing/JEditorPane/4694598/FrameContent.html line 1:
> 1:
Can this file be created by the test on the fly? There's no need for a
supporting file.
In #18147, Prasanta handled the same situation.
test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 29:
> 27: * @summary JEditor pane throws NullPointerException on mouse movement.
> 28: * @library ../../regtesthelpers
> 29: * @build JRobot
The test doesn't seem to use any of the extended functionality provided by
JRobot, so the standard `java.awt.Robot` can be used instead.
test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 40:
> 38:
> 39: public class bug4694598 {
> 40: JFrame frame = null;
Suggestion:
JFrame frame;
`null` is the default value any way.
test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 69:
> 67: public void performTest() throws InterruptedException,
> 68: InvocationTargetException {
> 69: JRobot jRobo = JRobot.getRobot();
Suggestion:
JRobot jRobo = JRobot.getRobot();
jRobo.waitForIdle();
Let the frame appear on the screen.
test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 81:
> 79: try {
> 80: Thread.sleep(50);
> 81: } catch (InterruptedException ex) {}
Suggestion:
jRobo.delay(50);
test/jdk/javax/swing/JEditorPane/4694598/bug4694598.java line 93:
> 91:
> 92: public static void main(String args[]) throws InterruptedException,
> 93: InvocationTargetException {
Suggestion:
public static void main(String[] args) throws Exception {
Java-style array declaration, shortened `throws` clause.
-
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18184#pullrequestreview-1927877446
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1519748640
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1519751913
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1519757762
PR Review Comment: https://git.openjdk.org/jdk/pull/18184#discussion_r1519759741
PR Review
Re: RFR: 8327787: Convert javax/swing/border/Test4129681.java applet test to main
On Mon, 11 Mar 2024 11:10:14 GMT, Tejesh R wrote:
> Convert javax/swing/border/Test4129681.java applet test to main based test
> using PassFailJFrame.
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/border/Test4129681.java line 48:
> 46: String testInstructions = """
> 47: When applet starts, you'll see a checkbox
> 48: and a label with a titled border.
Update the instructions not to refer to applet.
test/jdk/javax/swing/border/Test4129681.java line 54:
> 52: """;
> 53: PassFailJFrame passFailJFrame = new PassFailJFrame.Builder()
> 54: .title("JInternalFrame Instructions")
Is this test related to `JInternalFrame`? It doesn't look so.
test/jdk/javax/swing/border/Test4129681.java line 56:
> 54: .title("JInternalFrame Instructions")
> 55: .instructions(testInstructions)
> 56: .testTimeOut(5)
The default timeout could be left out.
test/jdk/javax/swing/border/Test4129681.java line 67:
> 65: }
> 66:
> 67: public static void init() {
I propose using `testUI` method of the builder, it hides the complexity of
registering and positioning the frame. Modify this method to return the created
`frame`.
See PR #17838 as an example or other PRs. The javadoc for `PassFailJFrame` also
contains the recommended usage.
-
PR Review: https://git.openjdk.org/jdk/pull/18189#pullrequestreview-1927850517
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519732701
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519733729
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519734220
PR Review Comment: https://git.openjdk.org/jdk/pull/18189#discussion_r1519743444
Re: RFR: 8075917: The regression-swing case failed as the text on label is not painted red with the GTK L&F [v5]
On Thu, 7 Mar 2024 12:42:05 GMT, Abhishek Kumar wrote:
>>> Backtracking a bit, regarding `Label.foreground` why it needs to be done
>>> here?
>>>
>>> Other L&F, it is added in corresponding LookAndFeel class for example, in
>>> [Metal](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/plaf/metal/MetalLookAndFeel.java#L1001)
>>> and in
>>> [Windows](https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/WindowsLookAndFeel.java#L880)
>>> etc so I think it is needed to be added in GTKLookAndFeel class
>>
>> IN GTKLookAndFeel, the color is returned specific to the style and state
>> from native side . So, even if it is not added the default color for
>> enabled, disabled or any other state is returned by native. Whereas In Metal
>> the color is set by using UIManager define color.
>>
>>> and additionally there is setting of Label.foreground and other Label.
>>> property in BasicLabelUI.java
>>> [installDefaults](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicLabelUI.java#L368)
>> but we dont do it for Nimbus or GTK or Synth so I guess we need to call
>> super.installDefaults(c) in SynthLabelUI class since there is no GTKLabelUI
>> class
>> or
>> maybe do
>> LookAndFeel.installColorsAndFont(c, "Label.background", "Label.foreground",
>> "Label.font"); alone to avoid other properties being set and to not fallback
>>
>> This properties are set via SynthStyle
>> [installDefaults](https://github.com/kumarabhi006/jdk/blob/master/src/java.desktop/share/classes/javax/swing/plaf/synth/SynthStyle.java#L928)
>> method where the foreground, background are set by getting the value from
>> `getColorForState` method. So, calling super.installDefaults(c) in
>> SynthLabelUI will not make any difference.
>>
>> I checked for JLabel properties set in enabled and disabled state.
>>
>>
>> In Nimbus LAF
>> UIManager.getDefaults().put("Label[Enabled].textForeground",
>> labelColor);
>> UIManager.getDefaults().put("Label[Disabled].textForeground",
>> labelDisabledColor);
>>
>> Other LAF
>> UIManager.getDefaults().put("Label.foreground", labelColor);
>> UIManager.getDefaults().put("Label.disabledForeground",
>> labelDisabledColor);
>>
>>
>> It works as expected for Metal and GTK. For Nimbus LAF, disabled color is
>> not painted correctly. In stead the default color specified in skin.laf is
>> rendered.
>
>> It works as expected for Metal and GTK. For Nimbus LAF, disabled color is
>> not painted correctly. In stead the default color specified in skin.laf is
>> rendered.
>
> For HTML text, disabled state doesn't render in LAF defined color for Metal
> and GTK as well.
This looks weird… So you're saying `Label[Enabled].textForeground` and
`Label[Disabled].textForeground` are used for Nimbus (and Synth and GTK)
instead of `Label.foreground` and `Label.disabledForeground` which are used for
other L&Fs.
Shouldn't we fix the problem by correcting the keys instead? It looks like it's
what you're doing for specific components.
Is it specified anywhere that Synth-based L&Fs use different constants? It
results in incorrect colors.
If a developer sets the common properties, should they override Look-and-Feel
defaults?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17763#discussion_r1519696320
Re: RFR: 8075917: The regression-swing case failed as the text on label is not painted red with the GTK L&F [v5]
On Tue, 5 Mar 2024 05:28:01 GMT, Abhishek Kumar wrote:
>> JLabel text is not painted with the LAF defined foreground color in GTK LAF.
>> In GTK LAF the foreground color is retrieved by using native system APIs.
>> Fix is to return the foreground color if it is set by LAF defined property
>> otherwise return the default color by calling native APIs.
>> Applet based test has been converted to automatic test and check for all
>> installed LAFs. CI testing is green for test suite and individual test. Link
>> attached in JBS.
>
> Abhishek Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> separate method to get LAF defined color
src/java.desktop/share/classes/com/sun/java/swing/plaf/gtk/GTKStyle.java line
186:
> 184:
> 185: if (id == Region.LABEL && type == ColorType.FOREGROUND
> 186: && (state & SynthConstants.ENABLED) != 0) {
Doesn't `SynthConstants.DISABLED` need the same treatment?
test/jdk/javax/swing/plaf/basic/BasicHTML/bug4248210.java line 111:
> 109:
> 110: private static boolean chkImgForegroundColor(BufferedImage img) {
> 111: Color red = new Color(255, 0, 0);
I believe you should be using `labelColor` here. If anyone changes its value
from `Color.RED` to anything else, the test will fail but it shouldn't. After
all, why allowing configuring the colour in first place.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17763#discussion_r1519714194
PR Review Comment: https://git.openjdk.org/jdk/pull/17763#discussion_r1519712200
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v15]
On Sat, 9 Mar 2024 00:54:05 GMT, Alisen Chung wrote: >> Root cause of the test failure was fixed with >> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the >> other fix also included a test update. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > update test instructions, fix spacing test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 70: > 68:"The test checks if any exceptions are thrown when > removing and\n" + > 69:" re-adding the icon. If something is wrong, the > test will automatically fail.\n" + > 70:" Repeat clicks several times Then press PASS > button."; You've lost the full stop after _“times”_. It's not worth a new PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1519541474
Re: RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class [v2]
On Fri, 8 Mar 2024 11:34:12 GMT, Prasanta Sadhukhan wrote: >> OPensource BoxView test > > Prasanta Sadhukhan has updated the pull request incrementally with one > additional commit since the last revision: > > Review comment update Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1927533498
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS [v2]
On Thu, 7 Mar 2024 12:36:05 GMT, Dmitry Markov wrote: >> Updated several tests to avoid potential failure with recent LCMS update and >> non-LCMS generated profile. > > Dmitry Markov has updated the pull request incrementally with one additional > commit since the last revision: > > Tolerance update It's not mentioned here, however, I think it's relevant to the discussion: the updated tests pass when the non-LCMS profiles from Java 8 are used. - PR Comment: https://git.openjdk.org/jdk/pull/18097#issuecomment-1988171424
Re: RFR: 8326661: sun/java2d/cmm/ColorConvertOp/ColConvTest.java assumes profiles were generated by LCMS [v2]
On Thu, 7 Mar 2024 12:36:05 GMT, Dmitry Markov wrote: >> Updated several tests to avoid potential failure with recent LCMS update and >> non-LCMS generated profile. > > Dmitry Markov has updated the pull request incrementally with one additional > commit since the last revision: > > Tolerance update Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18097#pullrequestreview-1927516502
Re: RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
On Thu, 7 Mar 2024 03:16:20 GMT, Prasanta Sadhukhan
wrote:
> OPensource BoxView test
Marked as reviewed by aivanov (Reviewer).
test/jdk/javax/swing/text/BoxView/bug6494356.java line 35:
> 33:
> 34: import java.awt.Dimension;
> 35: import java.awt.Toolkit;
Usually imports are in alphabetical order without giving special treatment to
`java.nio` and `java.io` packages.
test/jdk/javax/swing/text/BoxView/bug6494356.java line 51:
> 49:
> 50: public class bug6494356 {
> 51: static String testSrc = System.getProperty("test.src", ".");
`testSrc` is now unused.
-
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1924415667
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1517439785
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1517440328
Re: RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
On Thu, 7 Mar 2024 14:18:39 GMT, Tejesh R wrote: >> It's not applet-based. The HTML file is an argument to the test, it is the >> file that's loaded into `JEditorPane`. > > Oh, my bad, didn't notice it. Yeah, it's a bit confusing… - PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516237829
Re: RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
On Thu, 7 Mar 2024 04:53:07 GMT, Tejesh R wrote: >> OPensource BoxView test > > test/jdk/javax/swing/text/BoxView/bug6494356.java line 28: > >> 26: * @key headful >> 27: * @summary Test that BoxView.layout() is not called with negative >> arguments >> 28: * @run main bug6494356 bug6494356.html > > Can the test be converted to non-applet based? It's not applet-based. The HTML file is an argument to the test, it is the file that's loaded into `JEditorPane`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516193959
Re: RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class
On Thu, 7 Mar 2024 03:16:20 GMT, Prasanta Sadhukhan
wrote:
> OPensource BoxView test
Changes requested by aivanov (Reviewer).
Changes requested by aivanov (Reviewer).
Changes requested by aivanov (Reviewer).
test/jdk/javax/swing/text/BoxView/bug6494356.html line 1:
> 1: Paragraph
Since the test requires a supporting file, does it make sense to put the test
in a folder `6494356/bug6494356.java`?
I wanted to asked whether an external file is needed? I answered this question:
yes, it's needed. The test must use `setPage` which loads the file or URL
asynchronously.
At the same time, the file to load can be created on the fly in the scratch
directory which is the current directory when the test is run by jtreg.
Thus, an additional supporting file can be removed. Could update the test to
create the file as a temporary file, please?
test/jdk/javax/swing/text/BoxView/bug6494356.java line 34:
> 32: import java.beans.PropertyChangeEvent;
> 33: import java.beans.PropertyChangeListener;
> 34: import java.io.IOException;
`IOException` is unused.
test/jdk/javax/swing/text/BoxView/bug6494356.java line 56:
> 54: public void run() {
> 55: ep = new JEditorPane();
> 56: ep.setEditorKitForContentType("text/html", editorKit);
Suggestion:
ep.setEditorKitForContentType("text/html", new MyEditorKit());
The instance of the custom `EditorKit` can be created here. I think it makes
the test clearer.
test/jdk/javax/swing/text/BoxView/bug6494356.java line 72:
> 70: f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
> 71: f.setVisible(true);
> 72: File file = new File("file:" + testSrc + "/" +
> "bug6494356.html");
Where does this file come from?
As the first step, you should write the file into *the current directory* with
the content of `bug6494356.html`, that is `Paragraph`.
final Path file = Path.of("bug6494356.html");
try (Writer writer = Files.newBufferedWriter(file)) {
writer.write("Paragraph");
}
Then you load the file:
ep.setPage("file:" + file);
In the end of the test, you may remove the created file:
Files.delete(file);
test/jdk/javax/swing/text/BoxView/bug6494356.java line 79:
> 77: try (Writer writer = Files.newBufferedWriter(file)) {
> 78: writer.write("Paragraph");
> 79: } catch (Exception e) {
I suggest creating the file *before* calling `SwingUtilities.invokeLater`.
If it throws an exception, the test fails right away, you won't even run the
code inside `invokeLater`.
test/jdk/javax/swing/text/BoxView/bug6494356.java line 117:
> 115: }
> 116:
> 117: static ViewFactory viewFactory = new MyViewFactory();
This should be a `final` field of `MyEditorKit`, no need to make it `static`.
-
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1922519738
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1922960768
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1923181380
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516212830
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516209847
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516204845
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516478899
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516606920
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516208607
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v14]
On Thu, 7 Mar 2024 19:22:06 GMT, Alisen Chung wrote:
>> Root cause of the test failure was fixed with
>> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the
>> other fix also included a test update.
>
> Alisen Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> move jtreg in ShowAfterDisposeTest, remove trayicon in
> ShowAfterDisposeTest, changed instructions string in DisposeInActionEventTest
Marked as reviewed by aivanov (Reviewer).
test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java
line 69:
> 67:" the tray and then added back in a second.\n\n" +
> 68:"If something is wrong, the test will
> automatically fail.\n" +
> 69:" Repeat clicks several times. Press PASS
> button.";
Suggestion:
" Repeat clicks several times. Then press PASS button.";
Does it sound better this way?
test/jdk/java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.java line
46:
> 44: public class ShowAfterDisposeTest {
> 45: private static SystemTray tray;
> 46: private static TrayIcon icon ;
Suggestion:
private static TrayIcon icon;
-
PR Review: https://git.openjdk.org/jdk/pull/17838#pullrequestreview-1923688183
PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516922486
PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516921701
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v11]
On Thu, 7 Mar 2024 15:09:02 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> null check for system tray before remove > > test/jdk/java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.java > line 1: > >> 1: /* > > Does it make sense to remove the tray icon in this test as well when the test > exits? How is this resolved? You haven't updated `ShowAfterDisposeTest.java` to remove the tray icon when the test exists. An answer of ‘No‘ is also valid. But you haven't said anything at all. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516573259
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v13]
On Thu, 7 Mar 2024 17:06:11 GMT, Alisen Chung wrote:
>> Root cause of the test failure was fixed with
>> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the
>> other fix also included a test update.
>
> Alisen Chung has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - remove trayicon when feiling test
> - remove automatic check line in instructions
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java
line 117:
> 115: if (systemTray != null) {
> 116: systemTray.remove(trayIcon);
> 117: }
It is not needed. The icon is always removed, the `finally` block in the `main`
method is always executed, even when an exception is thrown.
https://github.com/openjdk/jdk/blob/4bc3afc11a8a88599bc70fc7974b4f0782c60b2c/test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java#L81-L85
If it were needed, you would create a helper method and call it from all the
places instead of copying the code around.
-
PR Review: https://git.openjdk.org/jdk/pull/17838#pullrequestreview-1923090600
PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516558328
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v8]
On Thu, 7 Mar 2024 17:00:29 GMT, Alisen Chung wrote: >> This comment also implied amending the instructions to correspond to what >> the test does. > > the test doesn't actually do any automatic checks, so i've removed that line It kind of does. [JDK-6299866](https://bugs.openjdk.org/browse/JDK-6299866) states NPE was thrown if a tray icon is removed from its action listener. This implies the test fails automatically if NPE is thrown. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516561499
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v11]
On Thu, 7 Mar 2024 16:55:12 GMT, Christoph Langer wrote: >> The assertions reported in the bug were observed spuriously and here and >> there broke tests in some Windows configurations. >> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129), >> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or >> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to >> this. >> >> The problem is that in Windows environments without a valid display, e.g. >> started by system services or via PowerShell Remoting, one sees a Monitor >> with name 'Windisc' in `EnumDisplayMonitors`. >> However, it seems to be some kind of a pseudo device where you can not get a >> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well >> documented, though. >> >> I hereby modify the device initialization code to only count/detect monitors >> where CreateDC returns non-NULL in Devices.cpp. I also add some more >> checking/error handling to AwtWin32GraphicsDevice::Initialize() for >> correctness. >> >> Furthermore, I re-enable the test >> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows >> Debug VMs, which reverts the fix from JDK-8269529 that should not be >> necessary any more. > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Bring back verification of hBMDC and hBM Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17614#pullrequestreview-1923069535
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v9]
On Thu, 7 Mar 2024 16:48:27 GMT, Christoph Langer wrote: >> If you don't add the assertions for `hBMDC` and `hBM`, you should revert all >> the changes to `awt_Win32GraphicsDevice.cpp`. >> >> At the moment, two lines are removed from `initScreens`. > > The two lines removed from initScreens are in `awt_Win32GraphicsEnv.cpp` 😉 > That's unrelated cleanup I guess. But I bring back the VERIFY code which > seems to make sense and would not alter the behavior in the non-debug case. You're right! `awt_Win32GraphicsDevice.cpp` is not part of the PR at this very moment. - PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1516508137
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v9]
On Thu, 7 Mar 2024 16:39:03 GMT, Alexey Ivanov wrote: >> Christoph Langer has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 14 additional >> commits since the last revision: >> >> - declare variable hasDisplays final >> - Merge branch 'master' into JDK-8185862 >> - load awt.dll before display check >> - Change implementation of headless determination on Windows >> - Merge branch 'master' into JDK-8185862 >> - Reflect display detection in java.awt.GraphicsEnvironment::isHeadless() >> - Merge branch 'master' into JDK-8185862 >> - Get rid of global variables and restore old handling wrt calling >> ::GetDIBits >> - Merge branch 'master' into JDK-8185862 >> - Little cleanup >> - ... and 4 more: https://git.openjdk.org/jdk/compare/dcf260b9...002fba2d > > src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp > line 182: > >> 180: gpBitmapInfo->bmiHeader.biBitCount = 0; >> 181: HDC hBMDC = this->GetDC(); >> 182: VERIFY(hBMDC != NULL); > > You may want to leave `VERIFY(hBMDC != NULL)`… just in case. If you don't add the assertions for `hBMDC` and `hBM`, you should revert all the changes to `awt_Win32GraphicsDevice.cpp`. At the moment, two lines are removed from `initScreens`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1516495998
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v9]
On Wed, 6 Mar 2024 07:50:08 GMT, Christoph Langer wrote: >> The assertions reported in the bug were observed spuriously and here and >> there broke tests in some Windows configurations. >> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129), >> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or >> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to >> this. >> >> The problem is that in Windows environments without a valid display, e.g. >> started by system services or via PowerShell Remoting, one sees a Monitor >> with name 'Windisc' in `EnumDisplayMonitors`. >> However, it seems to be some kind of a pseudo device where you can not get a >> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well >> documented, though. >> >> I hereby modify the device initialization code to only count/detect monitors >> where CreateDC returns non-NULL in Devices.cpp. I also add some more >> checking/error handling to AwtWin32GraphicsDevice::Initialize() for >> correctness. >> >> Furthermore, I re-enable the test >> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows >> Debug VMs, which reverts the fix from JDK-8269529 that should not be >> necessary any more. > > Christoph Langer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - declare variable hasDisplays final > - Merge branch 'master' into JDK-8185862 > - load awt.dll before display check > - Change implementation of headless determination on Windows > - Merge branch 'master' into JDK-8185862 > - Reflect display detection in java.awt.GraphicsEnvironment::isHeadless() > - Merge branch 'master' into JDK-8185862 > - Get rid of global variables and restore old handling wrt calling > ::GetDIBits > - Merge branch 'master' into JDK-8185862 > - Little cleanup > - ... and 4 more: https://git.openjdk.org/jdk/compare/dcf260b9...002fba2d src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp line 182: > 180: gpBitmapInfo->bmiHeader.biBitCount = 0; > 181: HDC hBMDC = this->GetDC(); > 182: VERIFY(hBMDC != NULL); You may want to leave `VERIFY(hBMDC != NULL)`… just in case. - PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1516489473
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v10]
On Thu, 7 Mar 2024 16:35:11 GMT, Christoph Langer wrote: >> The assertions reported in the bug were observed spuriously and here and >> there broke tests in some Windows configurations. >> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129), >> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or >> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to >> this. >> >> The problem is that in Windows environments without a valid display, e.g. >> started by system services or via PowerShell Remoting, one sees a Monitor >> with name 'Windisc' in `EnumDisplayMonitors`. >> However, it seems to be some kind of a pseudo device where you can not get a >> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well >> documented, though. >> >> I hereby modify the device initialization code to only count/detect monitors >> where CreateDC returns non-NULL in Devices.cpp. I also add some more >> checking/error handling to AwtWin32GraphicsDevice::Initialize() for >> correctness. >> >> Furthermore, I re-enable the test >> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows >> Debug VMs, which reverts the fix from JDK-8269529 that should not be >> necessary any more. > > Christoph Langer has updated the pull request incrementally with one > additional commit since the last revision: > > Revert touching awt_Win32GraphicsDevice.cpp altogether and remove > unnecessary private method for hasDisplays() Marked as reviewed by aivanov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17614#pullrequestreview-1922978004
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v11]
On Thu, 7 Mar 2024 16:30:32 GMT, Alisen Chung wrote: >> test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java >> line 63: >> >>> 61:" (also called Taskbar Status Area on MS >>> Windows, Notification\n" + >>> 62:" Area on Gnome or System Tray on KDE) is >>> visible.\n\n" + >>> 63: clickInstruction + " the button on the tray >>> icon to trigger the\n" + >> >> What I see on Windows: “Double click _the button_ on the tray icon to >> trigger the…” >> >> What button do I need to click? > > On windows you need to left click twice, on macos you need to right click once [I did suggest clearer instructions](https://github.com/openjdk/jdk/pull/17838#discussion_r1506056726): clickInstruction + " on the tray icon to trigger the\n" + You ignored it and marked resolved. In the current instructions, _“the button”_ does not refer to anything obvious. What button? It's confusing. Remove _“the button”_ from the instructions, and the instructions are clear: _“Right-click / Double-click on **the tray icon**…”_ - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516487194
Re: RFR: 8309460: JScrollBar leaves behind clutter for non-integer UI scales [v2]
On Thu, 22 Feb 2024 05:50:11 GMT, Tejesh R wrote: > The test can be converted to automatic by comparing the captured scroll area > before MousePress and between MousePress and Release events. I agree, the test can be automatic. Grab the thumb with robot and move to the right edge. Without releasing the left mouse button¹, create a multiresolution screenshot to get all the pixels without scaling the image down. The background of the scrollbar on the captured screenshot should not contain darker pixels. ¹ After you release the mouse button, the position of the thumb is adjusted, and the entire scrollbar is repainted. At which point, the leftover darker lines disappear from the scrollbar background. - PR Comment: https://git.openjdk.org/jdk/pull/17484#issuecomment-1983889766
Re: RFR: 8309460: JScrollBar leaves behind clutter for non-integer UI scales
On Wed, 24 Jan 2024 21:11:25 GMT, Alexey Ivanov wrote: > > > Because float scale results in fractional pixels? > > > If the scale is 1.25, then 1 pixel is 1.25 physical pixels; 2 pixels are > > > 2.5 physical pixels and so on. > > > > Initially this issue looked similar to fractional scaling issues such as > > border rendering issues observed on windows previously but it seems to > > happen only on non-standard fractional UI scales such as 2.3, 2.7 and not > > on standard fractional scales supported on windows (1.25,1.75,2.25 ..) > > [#17484 > > (review)](https://github.com/openjdk/jdk/pull/17484#pullrequestreview-1834275178). > > The underlying problem is still the same: there are fractional pixels. I ran the test now and I'm sure the problem is very similar. When drag the thumb, the width of the left border is usually 2 pixels, sometimes it changes to 3 pixels. As I move farther, that third pixel remains on the background of the scrollbar. The left over pixels create a regular pattern: 17 pixels followed by 8 pixels, then the cycle repeats. These are physical display pixels. If you convert user space pixels to physical pixels, you'll see there's a pattern where fraction adds up to whole pixel: 4 user pixels, 3 user pixels. As you drag the thumb, you can see that the highlight/shadow lines on the thumb itself dance. This means that as you drag the thumb, the app sees 1-pixel increments which results in 2-pixel and 3-pixel increments on the screen. > How it related to the "non-standard" fractional metrics(such as 2.3, 2.7) and > why it works fine for 1.5 and 1.75. I believe I explained how it's related to fractional scales. Why does it work for 1.25 and 1.75? I guess we never end up in a situation where a physical pixel doesn't get painted. With 1.5, the probability is even less. You can still see the width of the left (and right) border changes; the highlight and shadow lines on the thumb continue to dance. There are this kind of problems wherever pixels are painted by 1-pixel lines. Look at the title bar of a JInternalFrame in SwingSet2 as drag it: the pattern changes even at 1.5 scale; dragging at 1.25 and 1.75 gives another pattern. > If we draw a border outside of the component that border should be fixed. If > the clear/fillRect does not clear the component then we should fix the > calculation of the bounds. No, we don't draw outside of component bounds. We just don't repaint enough pixels as thumb is moved. As you rightfully noticed something different is needed. Repainting the bounds of the thumb still solves the problem but it doesn't seem right. As the thumb is dragged, it's repainted in the new location; the old location is erased eventually. When erasure is handled, I'd adjust the left coordinate to be (left - 1) to repaint a larger rectangle or something like this. - PR Comment: https://git.openjdk.org/jdk/pull/17484#issuecomment-1983874053
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v11]
On Wed, 6 Mar 2024 21:36:10 GMT, Alisen Chung wrote: >> Root cause of the test failure was fixed with >> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the >> other fix also included a test update. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > null check for system tray before remove Changes requested by aivanov (Reviewer). test/jdk/java/awt/TrayIcon/ShowAfterDisposeTest/ShowAfterDisposeTest.java line 1: > 1: /* Does it make sense to remove the tray icon in this test as well when the test exits? - PR Review: https://git.openjdk.org/jdk/pull/17838#pullrequestreview-1922717083 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516328831
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v8]
On Tue, 27 Feb 2024 11:01:50 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove system property set > > test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java > line 67: > >> 65:" something is wrong the corresponding message >> is displayed below.\n" + >> 66:" Repeat clicks several times. If no 'Test >> FAILED' messages\n" + >> 67:" are printed, press PASS button else FAIL >> button."; > > It's not displayed below, it's to the right, isn't it? > > Fail the test automatically if you can detect the failure. You may show a > message to the tester about the failure before shutting down the test. This comment also implied amending the instructions to correspond to what the test does. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516304874
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v8]
On Wed, 28 Feb 2024 14:29:24 GMT, Alexey Ivanov wrote: >> test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java >> line 60: >> >>> 58:" (also called Taskbar Status Area on MS >>> Windows, Notification\n" + >>> 59:" Area on Gnome or System Tray on KDE) is >>> visible.\n\n" + >>> 60: clickInstruction + " the button on the tray >>> icon to trigger the\n" + >> >> _“Right-click the button”_ or _“Double-left click the button”_ doesn't make >> sense to me. > > The recent update didn't clarify it, now the text reads _“Right-click the > button”_ or _“Double click the button”_. > > I assume _“the button”_ refers to the mouse button. But it's very confusing, > it's not clear what is required; because “right-click” and “double-click” > already define which _mouse button_ to use. > > What you want to say is _“Right-click / Double-click on **the tray icon**…”_ It is not resolved. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516284965
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v10]
On Wed, 6 Mar 2024 20:47:53 GMT, Harshitha Onkar wrote:
>> Alisen Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> implemented changes from feedback
>
> test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java
> line 84:
>
>> 82: } finally {
>> 83: systemTray.remove(trayIcon);
>> 84: }
>
> @alisenchung I missed adding null check here earlier. It is good to have this
> safety check to avoid NPE.
>
> Suggestion:
>
> } finally {
> if (systemTray != null) {
>systemTray.remove(trayIcon);
> }
> }
`SystemTray.getSystemTray()` can't return `null`. However, the static field
`systemTray` can remain `null` in the test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516326933
Re: RFR: 8327492: Remove applet usage and update DisposeInActionEventTest.html [v11]
On Wed, 6 Mar 2024 21:36:10 GMT, Alisen Chung wrote: >> Root cause of the test failure was fixed with >> https://bugs.openjdk.org/browse/JDK-8316931, updating this test since the >> other fix also included a test update. > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > null check for system tray before remove test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 63: > 61:" (also called Taskbar Status Area on MS Windows, > Notification\n" + > 62:" Area on Gnome or System Tray on KDE) is > visible.\n\n" + > 63: clickInstruction + " the button on the tray icon > to trigger the\n" + What I see on Windows: “Double click _the button_ on the tray icon to trigger the…” What button do I need to click? test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 65: > 63: clickInstruction + " the button on the tray icon > to trigger the\n" + > 64:" action event. Brief information about action > events is printed\n" + > 65:" in the frame. After each action event the tray > icon is removed from\n" + Suggestion: " in the frame. After each action event, the tray icon is removed from\n" + test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 68: > 66:" the tray and then added back in a second.\n\n" + > 67:"The test performs some automatic checks when > removing the icon. If\n" + > 68:" something is wrong the corresponding message > the test will automatically fail.\n" + Suggestion: " something is wrong, the test will automatically fail.\n" + test/jdk/java/awt/TrayIcon/DisposeInActionEventTest/DisposeInActionEventTest.java line 70: > 68:" something is wrong the corresponding message > the test will automatically fail.\n" + > 69:" Repeat clicks several times. If no 'Test > FAILED' messages\n" + > 70:" are printed, press PASS button else FAIL > button."; Suggestion: "\n" + "Repeat clicks several times. Press PASS button."; Perhaps, the instructions need to be re-worked further. The tester needs to perform the action several times. The test will fail automatically if anything's wrong. If it doesn't, press the Pass button. - PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516286794 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516287544 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516288497 PR Review Comment: https://git.openjdk.org/jdk/pull/17838#discussion_r1516295653
Re: RFR: 8320676 : Manual printer tests have no Pass/Fail buttons, instructions close set 1 [v11]
On Tue, 5 Mar 2024 12:13:02 GMT, Alexey Ivanov wrote: >> Renjith Kannath Pariyangad has updated the pull request incrementally with >> one additional commit since the last revision: >> >> Suggesions incorporated > > test/jdk/java/awt/print/PrinterJob/PrintAllFonts.java line 45: > >> 43: private static final int FONT_SIZE = 14; >> 44: private final Font[] allFonts = >> 45: >> GraphicsEnvironment.getLocalGraphicsEnvironment().getAllFonts(); > > I suggest adding blank lines to visually separate fields / constants with > different purpose. > Suggestion: > > private static final int LINE_HEIGHT = 18; > private static final int FONT_SIZE = 14; > > private final Font[] allFonts = > GraphicsEnvironment.getLocalGraphicsEnvironment().getAllFonts(); What about this? - PR Review Comment: https://git.openjdk.org/jdk/pull/17607#discussion_r1514715232
Re: RFR: 8324808 : Manual printer tests have no Pass/Fail buttons, instructions close set 3 [v9]
On Tue, 5 Mar 2024 14:56:22 GMT, Alexey Ivanov wrote: >> Renjith Kannath Pariyangad has updated the pull request incrementally with >> one additional commit since the last revision: >> >> Suggesions added > > test/jdk/java/awt/print/PrinterJob/PrintImage.java line 57: > >> 55: "Printing must be done in Japanese 2nd Edition.\n" + >> 56: "Preferably Canon LaserShot A309GII.\n" + >> 57: "\n" + > > Suggestion: > > > I believe we can remove the first part of the instructions. The original bug > [JDK-4298489](https://bugs.openjdk.org/browse/JDK-4298489) says printing by > the first method was ugly and the second one was good. Both methods should > produce the identical results. > > According to the bug description, it was reported against Windows 95 OSR 2 > although the instructions in the test refer to Windows 98 Second Edition and > its Japanese version for whatever reason. What about this comment? - PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1514684555
Re: RFR: 8324808 : Manual printer tests have no Pass/Fail buttons, instructions close set 3 [v11]
On Wed, 6 Mar 2024 09:46:58 GMT, Renjith Kannath Pariyangad wrote: >> Hi Reviewers, >> >> Updated manual printer test cases with 'PassFailJFrame', also removed unused >> variables. Added 'SkippedException' in case of printer missing or not >> configured. >> >> Please review and let me know your suggestions. >> >> Regards, >> Renjith > > Renjith Kannath Pariyangad has updated the pull request incrementally with > one additional commit since the last revision: > > Fixed comple issue There are quite a few other comments unresolved comments. test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 40: > 38: * @requires os.family=="windows" > 39: * @library /test/lib /java/awt/regtesthelpers > 40: * @build PassFailJFrame jtreg.SkippedException Suggestion: * @library /test/lib /java/awt/regtesthelpers * @build jtreg.SkippedException PassFailJFrame Change the order to align libraries and classes from the libraries? - Changes requested by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17609#pullrequestreview-1920039700 PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1514664523
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v2]
On Wed, 6 Mar 2024 10:43:16 GMT, Tejesh R wrote:
>> Alexey Ivanov has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Replace synchronized invalidateFileCache with synchronized block inside
>> - Declare DoChangeContents constructor private, wrap its parameters
>> - Space after synchronized in DoChangeContents.run
>> - Convert runnable to local variable
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
> line 552:
>
>> 550: if (remSize > 0 && addSize == 0) {
>> 551: fireIntervalRemoved(BasicDirectoryModel.this, remStart,
>> remStart + remSize - 1);
>> 552: } else if (addSize > 0 && remSize == 0 && addStart +
>> addSize <= fileCache.size()) {
>
> Any reason for moving this portion out of `Synchronized` block? Because
> `fileCache.size()` might need to be inside the `Synchronized` block right?
I didn't touch it, it never was inside the synchronized block. Only indentation
was changed in this method.
However, you're right it's better to store the size of `fileCache` inside the
synchronized block. The lock should not be held while notifying listeners.
On the other hand, `fileCache` can't change. The only place where `fileCache`
is modified is above in this method, and only on EDT. The EDT will remain busy
until a `fire*` method returns.
I've updated the code to avoid any confusion. Thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1514311500
Re: RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v3]
> Ensure access to the `filesLoader` field of `BasicDirectoryModel` is > synchronized. > > Without synchronization, a thread checks if `filesLoader` is not null and > creates a new `FilesLoader` thread. If the thread is pre-empted between these > two operations, another thread or even several threads can see the `null` > value and create new `FilesLoader` threads. > > The same way, `BasicDirectoryModel.invalidateFileCache` needs to be > synchornized to interrupt the current `filesLoader` and assign `null`. > > This bug allows reproducing `ConcurrentModificationException` seen in > [JDK-8323670](https://bugs.openjdk.org/browse/JDK-8323670) and > [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) using the test in > PR #18109. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Store the size of fileCache inside synchronized block - Changes: - all: https://git.openjdk.org/jdk/pull/18111/files - new: https://git.openjdk.org/jdk/pull/18111/files/a0659236..a9ec7f64 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18111&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18111&range=01-02 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18111/head:pull/18111 PR: https://git.openjdk.org/jdk/pull/18111
Re: RFR: 8324807 : Manual printer tests have no Pass/Fail buttons, instructions close set 2 [v10]
On Wed, 6 Mar 2024 06:07:20 GMT, Renjith Kannath Pariyangad
wrote:
>> test/jdk/java/awt/print/PrinterJob/Collate2DPrintingTest.java line 74:
>>
>>> 72: if (pj.printDialog(prSet)) {
>>> 73: pj.print(prSet);
>>> 74: }
>>
>> In this case, the print dialog has *Collated* checked and disabled; the
>> number of copies is set to 1 and I can't change the value.
>>
>> Does it work for you? Is it a bug in JDK?
>
> For print to pdf, yes not able to change number of copies, but for actual
> printer able to change the number.
Makes sense. I had only PDF printer. Thank you for confirming it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17608#discussion_r1514074678
Re: RFR: 8324808 : Manual printer tests have no Pass/Fail buttons, instructions close set 3 [v10]
On Wed, 6 Mar 2024 06:57:12 GMT, Renjith Kannath Pariyangad
wrote:
>> Hi Reviewers,
>>
>> Updated manual printer test cases with 'PassFailJFrame', also removed unused
>> variables. Added 'SkippedException' in case of printer missing or not
>> configured.
>>
>> Please review and let me know your suggestions.
>>
>> Regards,
>> Renjith
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with
> one additional commit since the last revision:
>
> Added Override
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/print/PrinterJob/PageDlgPrnButton.java line 54:
> 52: if (serviceCount < 2) {
> 53: throw new SkippedException("The test requires at least 2
> printers.");
> 54: }
This doesn't compile now. You didn't add the import for
`jtreg.SkippedException`, nor did you modify the jtreg test to compile
`SkippedException`.
test/jdk/java/awt/print/PrinterJob/PrintCompoundString.java line 43:
> 41: * @key printer
> 42: * @library /test/lib /java/awt/regtesthelpers
> 43: * @build PassFailJFrame jtreg.SkippedException
You added `SkippedException` to the test that doesn't need it.
-
PR Review: https://git.openjdk.org/jdk/pull/17609#pullrequestreview-1919123845
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1514068434
PR Review Comment: https://git.openjdk.org/jdk/pull/17609#discussion_r1514070364
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v9]
On Wed, 6 Mar 2024 07:50:08 GMT, Christoph Langer wrote:
>> The assertions reported in the bug were observed spuriously and here and
>> there broke tests in some Windows configurations.
>> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129),
>> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or
>> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to
>> this.
>>
>> The problem is that in Windows environments without a valid display, e.g.
>> started by system services or via PowerShell Remoting, one sees a Monitor
>> with name 'Windisc' in `EnumDisplayMonitors`.
>> However, it seems to be some kind of a pseudo device where you can not get a
>> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well
>> documented, though.
>>
>> I hereby modify the device initialization code to only count/detect monitors
>> where CreateDC returns non-NULL in Devices.cpp. I also add some more
>> checking/error handling to AwtWin32GraphicsDevice::Initialize() for
>> correctness.
>>
>> Furthermore, I re-enable the test
>> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows
>> Debug VMs, which reverts the fix from JDK-8269529 that should not be
>> necessary any more.
>
> Christoph Langer has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 14 additional
> commits since the last revision:
>
> - declare variable hasDisplays final
> - Merge branch 'master' into JDK-8185862
> - load awt.dll before display check
> - Change implementation of headless determination on Windows
> - Merge branch 'master' into JDK-8185862
> - Reflect display detection in java.awt.GraphicsEnvironment::isHeadless()
> - Merge branch 'master' into JDK-8185862
> - Get rid of global variables and restore old handling wrt calling
> ::GetDIBits
> - Merge branch 'master' into JDK-8185862
> - Little cleanup
> - ... and 4 more: https://git.openjdk.org/jdk/compare/e7e3e9ca...002fba2d
src/java.desktop/windows/classes/sun/awt/PlatformGraphicsInfo.java line 69:
> 67: public static boolean getDefaultHeadlessProperty() {
> 68: // If we don't find usable displays, we run headless.
> 69: return !hasDisplays();
I wonder if redirection via a private method makes any meaningful difference
compared to returning the value of the `hasDisplays` flag directly?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1514026459
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v8]
On Wed, 6 Mar 2024 07:49:35 GMT, Christoph Langer wrote:
> > It looks good to me.
> > The only question I have is for the fallback in
> > `awt_Win32GraphicsDevice.cpp`.
> > Bailing out quickly makes the code cleaner. In this case, if `::GetDIBits`
> > fails, we can bail out too.
> > If the fallback is needed, the code below the first call to `::GetDIBits`
> > should be executed even if `GetDC` and `CreateCompatibleBitmap` fail.
> > Since testing didn't reveal any problems, the fallback is probably not as
> > important, or not critical for a headless system.
>
> I think the bailout is unlikely to happen after this fix. At least not for
> the headless environment where we won't jump into
> AwtWin32GraphicsDevice::Initialize() at all any more. And in headless
> environments with working monitors, we would never bail out, I guess.
> However, the method could be cleaned up I guess. But that should also be done
> in another issue and by somebody who is more into the details of what's going
> on there.
In this case, I think you should update the code to bail out if `::GetDIBits`
fails. This way we will always bail out on any failure.
https://github.com/openjdk/jdk/blob/58a404443691b90439800fb2fd0f76b41667b707/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp#L192-L198
At the moment the code is inconsistent. If this code isn't even run in headless
environment, then it's better to make it consistent. Otherwise, it raises
questions why it returns if either if `GetDC` or `CreateCompatibleBitmap` fails
but continues if `::GetDIBits` fails.
>> src/java.desktop/windows/classes/sun/awt/PlatformGraphicsInfo.java line 38:
>>
>>> 36:
>>> 37: static {
>>> 38: loadAWTLibrary();
>>
>> Can `WToolkit.loadLibraries()` be used here? The method is declared public,
>> so it should be accessible.
>>
>> It may create a circular dependency though.
>
> Maybe. I think, as Phil already mentions, a cleanup/centralization of the
> places that load libawt should be done. But I would suggest to do this in a
> separate issue.
Sounds reasonable. Will you create a CR for this? So that we won't forget about
it.
-
PR Comment: https://git.openjdk.org/jdk/pull/17614#issuecomment-1980364103
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1514022885
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v2]
On Fri, 16 Feb 2024 08:45:59 GMT, Christoph Langer wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsDevice.cpp >> line 191: >> >>> 189: return; >>> 190: } >>> 191: VERIFY(::GetDIBits(hBMDC, hBM, 0, 1, NULL, gpBitmapInfo, >>> DIB_RGB_COLORS)); >> >> I think the return value of `::GetDIBits` should also be analysed explicitly. >> >> I would prefer backing out if any of the functions above including >> `::GetDIBits` returns an error. >> >> Yet I'm not sure how critical it is to run the code below. [As I >> mentioned](https://github.com/openjdk/jdk/pull/17197#issuecomment-1875562325) >> in PR #17197, there's a workaround for a case where `::GetDIBits` returns >> 0. @MBaesken implemented a fallback to run the code below if even if all the >> above calls fail. It may be safe to ignore … in headless environment. > > Hm, I added some code for verification of ::GetDIBits result. If it is 0, I > skip the rest of the coding. I don't want to touch more here since I don't > understand what it is doing. It is not what I meant… If we want to preserve the way it work, we should execute the below code even if `this->GetDC()` fails… On the other hand, testing didn't find any problems, in which case I prefer bailing out after `::GetDIBits` fails too. @prrace, Do you know how critical the code below is? Is it worth to code the fallback if the path to `::GetDIBits`? - PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1513292270
Re: RFR: 8185862: AWT Assertion Failure in ::GetDIBits(hBMDC, hBM, 0, 1, 0, gpBitmapInfo, 0) 'awt_Win32GraphicsDevice.cpp', at line 185 [v8]
On Fri, 1 Mar 2024 08:01:08 GMT, Christoph Langer wrote:
>> The assertions reported in the bug were observed spuriously and here and
>> there broke tests in some Windows configurations.
>> For instance [JDK-8266129](https://bugs.openjdk.org/browse/JDK-8266129),
>> [JDK-8269529](https://bugs.openjdk.org/browse/JDK-8269529) or
>> [JDK-8323664](https://bugs.openjdk.org/browse/JDK-8323664) came up due to
>> this.
>>
>> The problem is that in Windows environments without a valid display, e.g.
>> started by system services or via PowerShell Remoting, one sees a Monitor
>> with name 'Windisc' in `EnumDisplayMonitors`.
>> However, it seems to be some kind of a pseudo device where you can not get a
>> DC via `CreateDC`. This behavior/monitor type doesn't seem to be well
>> documented, though.
>>
>> I hereby modify the device initialization code to only count/detect monitors
>> where CreateDC returns non-NULL in Devices.cpp. I also add some more
>> checking/error handling to AwtWin32GraphicsDevice::Initialize() for
>> correctness.
>>
>> Furthermore, I re-enable the test
>> `javax/swing/reliability/HangDuringStaticInitialization.java` for Windows
>> Debug VMs, which reverts the fix from JDK-8269529 that should not be
>> necessary any more.
>
> Christoph Langer has updated the pull request incrementally with one
> additional commit since the last revision:
>
> load awt.dll before display check
It looks good to me.
The only question I have is for the fallback in `awt_Win32GraphicsDevice.cpp`.
Bailing out quickly makes the code cleaner. In this case, if `::GetDIBits`
fails, we can bail out too.
If the fallback is needed, the code below the first call to `::GetDIBits`
should be executed even if `GetDC` and `CreateCompatibleBitmap` fail.
Since testing didn't reveal any problems, the fallback is probably not as
important, or not critical for a headless system.
src/java.desktop/windows/classes/sun/awt/PlatformGraphicsInfo.java line 35:
> 33: public class PlatformGraphicsInfo {
> 34:
> 35: private static boolean hasDisplays;
I believe it can be declared `final`, it's initialised in the static
initialiser that follows the declaration.
Suggestion:
private static final boolean hasDisplays;
src/java.desktop/windows/classes/sun/awt/PlatformGraphicsInfo.java line 38:
> 36:
> 37: static {
> 38: loadAWTLibrary();
Can `WToolkit.loadLibraries()` be used here? The method is declared public, so
it should be accessible.
It may create a circular dependency though.
-
PR Review: https://git.openjdk.org/jdk/pull/17614#pullrequestreview-1917750109
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1513269942
PR Review Comment: https://git.openjdk.org/jdk/pull/17614#discussion_r1513267417
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
> The implementation of `Win32ShellFolder2.compareTo` is inconsistent: there > are cases where > `a < b & b < c but a == c` > which *violates its general contract*. > > In particular, it happens for the personal folder (*Documents*) if it is > listed *twice*: as a special and as a regular folder. > > The evaluation performed by the submitter of the bug provided enough details > to create a test, reproduce the problem and finally resolve it. > > Without the fix, the regression test always fails: > > a < b & b < c but a >= c > where > a = C:\Users\Documents(true) > b = C:\Users(false) > c = C:\Users\Documents(false) > > as well as for the reverse case: `a > b & b > c`. > > How it is possible to have the same folder in a list of files twice remains > unknown. I believe it is another bug in JDK, however, no one has been able to > reproduce it so far. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Handle fakePersonal on Windows 10 The desktop folder on Windows 10 does not include the Personal (Documents) folder. - Changes: - all: https://git.openjdk.org/jdk/pull/18126/files - new: https://git.openjdk.org/jdk/pull/18126/files/7c442382..7bccc847 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18126&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18126&range=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18126/head:pull/18126 PR: https://git.openjdk.org/jdk/pull/18126
RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent
The implementation of `Win32ShellFolder2.compareTo` is inconsistent: there are cases where `a < b & b < c but a == c` which *violates its general contract*. In particular, it happens for the personal folder (*Documents*) if it is listed *twice*: as a special and as a regular folder. The evaluation performed by the submitter of the bug provided enough details to create a test, reproduce the problem and finally resolve it. Without the fix, the regression test always fails: a < b & b < c but a >= c where a = C:\Users\Documents(true) b = C:\Users(false) c = C:\Users\Documents(false) as well as for the reverse case: `a > b & b > c`. How it is possible to have the same folder in a list of files twice remains unknown. I believe it is another bug in JDK, however, no one has been able to reproduce it so far. - Commit messages: - 8305072: Fix Win32ShellFolder2.compareTo - 8305072: Add test case for Win32ShellFolder2.compareTo Changes: https://git.openjdk.org/jdk/pull/18126/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18126&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305072 Stats: 156 lines in 2 files changed: 154 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18126/head:pull/18126 PR: https://git.openjdk.org/jdk/pull/18126
Re: RFR: 8286759: TextComponentPrintable: consequent -> consecutive positions [v2]
On Mon, 4 Mar 2024 18:10:04 GMT, Phil Race wrote: > And capitalise "We", and "In" since they begin sentences I agree, yet it's not always followed in the comments. > If we are going to fix that, we might as well fix the entire sentence. Thank you for your suggestion. The sentence didn't sound right to me. Yet I couldn't figure out the better way to phrase it even though I tried re-writing it. - PR Review Comment: https://git.openjdk.org/jdk/pull/18067#discussion_r1513061245
