Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:31:33 GMT, Andy Goryachev wrote: >> Changing `Util.shutdown()` to hide **all** showing Windows and then call >> `Platform.exit()`. >> This simplifies the tests and ensures a clean shutdown. > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > for each Marked as reviewed by kcr (Lead). - PR Review: https://git.openjdk.org/jfx/pull/1407#pullrequestreview-1948793872
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:34:17 GMT, Nir Lisker wrote: >> The only issue with this style is - it's hard to set breakpoints. >> In this case it's not that important. >> Thank you! > > It's a matter of style mostly, so I don't really mind it. The debug issue can > be remedied by adding breakpoints inside Lambdas, which all IDEs support I > think (Eclipse and IntelliJ for sure). "inside lambdas" - inside the referenced methods you mean? yes, but if that method is frequently called it's not what I want. There is no way to set a breakpoint at the moment of lambda invocation, I think, at least not in Eclipse. - PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531211388
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:28:54 GMT, Andy Goryachev wrote:
>> tests/system/src/test/java/test/util/Util.java line 383:
>>
>>> 381: runAndWait(() -> {
>>> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
>>> 383: w.hide();
>>
>> I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't
>> get modified (it's a one-time copy).
>> The iteration can also be
>>
>> List.copyOf(Window.getWindows()).forEach(Window::hide);
>>
>> if you want.
>
> The only issue with this style is - it's hard to set breakpoints.
> In this case it's not that important.
> Thank you!
It's a matter of style mostly, so I don't really mind it. The debug issue can
be remedied by adding breakpoints inside Lambdas, which all IDEs support I
think (Eclipse and IntelliJ for sure).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531207452
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
On Tue, 19 Mar 2024 22:23:46 GMT, Nir Lisker wrote:
>> Andy Goryachev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> for each
>
> tests/system/src/test/java/test/util/Util.java line 383:
>
>> 381: runAndWait(() -> {
>> 382: for (Window w : new ArrayList<>(Window.getWindows())) {
>> 383: w.hide();
>
> I think you can use `List.copyOf` instead of an `ArrayList` since it doesn't
> get modified (it's a one-time copy).
> The iteration can also be
>
> List.copyOf(Window.getWindows()).forEach(Window::hide);
>
> if you want.
The only issue with this style is - it's hard to set breakpoints.
In this case it's not that important.
Thank you!
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1407#discussion_r1531200964
Re: RFR: 8325566: [TestBug] Util.shutdown() to hide ALL Windows [v2]
> Changing `Util.shutdown()` to hide **all** showing Windows and then call > `Platform.exit()`. > This simplifies the tests and ensures a clean shutdown. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: for each - Changes: - all: https://git.openjdk.org/jfx/pull/1407/files - new: https://git.openjdk.org/jfx/pull/1407/files/2887d654..5773ebfd Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1407=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1407=00-01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1407.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1407/head:pull/1407 PR: https://git.openjdk.org/jfx/pull/1407
