On Wed, 12 Mar 2025 17:46:33 GMT, Alexey Ivanov <[email protected]> wrote:
>>> mainFrame = new JFrame("Bug 8033699 - 9 Tests for Grouped / Non-Grouped
>>> Radio Buttons");
>>
>> Makes sense… However, a generic title would be good enough. Something like
>> _“Radio button focus tests”_. The current title is too long, it doesn't fit
>> in the title bar of the frame (at least on Windows), therefore I see no
>> point in making it comprehensive and long.
>>
>>> I usually fix these when I touch the test anyway.
>>
>> In majority of cases, I do too. Yet I tend not to change lines that I don't
>> touch. From this point of view, additional changes aren't necessary — none
>> of the lines that don't fit into 80-column limit aren't touched.
>>
>> The problem I see with additional refactoring is that it adds noise to the
>> code review and it makes it harder to understand what the real, important
>> changes are.
>>
>> Use *your common sense*.
>>
>>> Please limit to 80 cols wherever applicable
>>
>> This is not applied strictly… I'm for following the 80-column limit where it
>> doesn't reduce the readability. Yet I'm for stronger enforcement of
>> 100-column limit. There are quite a few lines which are longer than 100
>> columns. The culprit is
>> `KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner()`
>> which accounts for 70 characters.
>>
>> I'd like to make it shorter, and the focus manager can be cached after the
>> first usage. At the same time, I'm unsure doing so in this code review is
>> reasonable.
>
> In short, I'd rather avoid doing additional refactoring, except for changing
> the frame title if Rajat wants to, because _none of the lines that need
> updating are **not** touched by the current changes_.
I have filed a new bug to incorporate the suggestions here and re-factor test
code separately - [JDK-8351884](https://bugs.openjdk.org/browse/JDK-8351884)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23964#discussion_r1992164170
