On Tue, 11 Mar 2025 22:52:39 GMT, Damon Nguyen <dngu...@openjdk.org> wrote:
>> Should these be done as part of this change or a separate bug for
>> refactoring /code cleanup of this test ?
>
> I usually fix these when I touch the test anyway.
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23964#discussion_r1992004341
