RFR: 8233177: Remove testcase for JDK-8001470 as fix has been reverted

Alexey Ivanov aivanov at openjdk.org
Wed Feb 14 11:02:53 UTC 2024


On Wed, 14 Feb 2024 09:08:59 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

> > For me, it fails on Windows because no suitable font is found;
> 
> It is not because of fonts availability...It's because of the order of `pack `and `setVisible `being called which is causing some issue with `i18nFieldView `layouting. If the `pack` is called **before** `setVisible` then it works in windows and other platforms, so one can see it's NOT because of font availability

It makes sense, actually: the square that's rendered instead of missing character takes some space, therefore the height shouldn't be zero.

> Now, in one of the test-sprint, it was mentioned that _setLocationRelativeTo followed by pack cause some issues_ so it is recommended to call `pack `before `setLocationRelativeTo` [#13633 (comment)](https://github.com/openjdk/jdk/pull/13633#discussion_r1178208629)

Yes, I noticed it… in some cases, and it's somewhat to be expected: the size of the frame affects the location.

> so in similar vein, I guess this can also argued that setVisible followed by pack can cause some issue and be recommended that `pack` should be called before `setVisible`

I agree, you should rather call `pack` or `setSize` before calling `setVisible`… but you're not required to.

There was a bug in `JScrollPane` which also reproduces only if `setVisible(true)` is called before `pack`. Here it is [JDK-4152524](https://bugs.openjdk.org/browse/JDK-4152524): ScrollPane AS_NEEDED always places scrollbars first time component is laid out.

When the test for JDK-4152524 was open-sourced, `pack()` was added and the test stopped reproducing the original problem. See [PR 14478, `ScrollPaneExtraScrollBar.java`](https://github.com/openjdk/jdk/pull/14478/files#diff-dea8716fbb80746e76aa78045e9d9041794791704396d49d9ad9050786ce0fc5L60)

The above means there's a workaround for the bug we're discussing, and the workaround follows the established best practices.

> > And this test demonstrates it. The test is not to be removed.
> 
> The fix and the number of regressions it caused (as mentioned in my previous comment) demonstrated that the understanding was not correct so why should we impose the test on the fixer of JDK-8001470..Let him/her devise a new testcase for the fix he/she proposes (if we still think pack can be called after setvisible but it can be argued why different thought process for order of setLocationRelativeTo/pack and setVisible/pack)

*The fix* caused regressions, *the fix* is backed out.

*The test* is still relevant, in my opinion: it demonstrates the problem described in the bug.

If and when a new fix for this bug is developed, the test can be used to verify the proposed fix. He or she will not need to write a new test (it's still needed to test the fix, isn't it?) and may still decide to modify the test or to write a new test if necessary.

I see no reason to remove the test.

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

PR Comment: https://git.openjdk.org/jdk/pull/17528#issuecomment-1943532972


More information about the client-libs-dev mailing list