On Wed, 12 Mar 2025 17:44:31 GMT, Alexey Ivanov <[email protected]> wrote:
>> 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.
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_.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23964#discussion_r1992007599
