RFR: 8286620: Create regression test for verifying setMargin() of JRadioButton [v3]

[Alexey Ivanov]

On Fri, 27 May 2022 17:21:29 GMT, Tejesh R <duke at openjdk.java.net> wrote:

>> Added test for checking setMargin() of JRadioButton.
>
> Tejesh R has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Updated based on review comments
>  - Added headful key

I wonder why the test is Windows-specific if it allows changing Look and Feels.

The only Look and Feel which ignores the background is Nimbus, other L&Fs on Windows at least paint the yellow and green background. As such, the test can be run on other platforms.

Was the original bug in Windows Look and Feel? If so, you may want to make it the default L&F.

The instructions don't mention anything about other Look and Feels. Does the tester have to verify each presented L&F?

test/jdk/javax/swing/JRadioButton/bug4380543.java line 65:

> 63:     static PassFailJFrame passFailJFrame;
> 64: 
> 65:     public static void main(String args[]) throws Exception {

Suggestion:

    public static void main(String[] args) throws Exception {

Avoid C-style array declarations.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 85:

> 83: 
> 84: class testFrame extends JFrame implements ActionListener {
> 85:     JPanel buttonsPanel;

`buttonsPanel` can be local variable, it's not used outside of `initComponents`.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 87:

> 85:     JPanel buttonsPanel;
> 86: 
> 87:     Map<String, String> lookAndFeelMaps = new HashMap<String, String>();

I think it should be `lookAndFeelMap`, in singular, it's one map. You can use diamond operator, that is drop type parameters on the right side. IDE usually suggests this.

Consider making the map final.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 94:

> 92: 
> 93:     public void initMap()
> 94:     {

The brace should be on the same line.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 106:

> 104: 
> 105:             lookAndFeelMaps.put(sMapKey, sLnF);
> 106:         }

Suggestion:

        for (UIManager.LookAndFeelInfo look : lookAndFeel) {
            lookAndFeelMaps.put(look.getName(), look.getClassName());
        }


I guess I mentioned it earlier. I think this way is cleaner and more user-friendly: the user will see the name of the Look and Feel rather than part of its class name.

The local variables `sLnF` and `sMapKey` declared above the look become unnecessary in this case.

If you use `LinkedHashMap`, the order of the L&Fs will be preserved the map will be iterated for creating the buttons.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 109:

> 107:     }
> 108: 
> 109:     public void initComponents() throws InterruptedException {

The `InterruptedException` is not thrown in the method, so the throws clause should be removed.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 128:

> 126:         getContentPane().add(buttonsPanel);
> 127: 
> 128:         for (Map.Entry mapElement : lookAndFeelMaps.entrySet()) {

Suggestion:

        for (Map.Entry<String, String> mapElement : lookAndFeelMaps.entrySet()) {


Use parametrized `Map.Entry`.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 157:

> 155:         String val = lookAndFeelMaps.get(key);
> 156: 
> 157:         setLookAndFeel(val);

Suggestion:

        setLookAndFeel(lookAndFeelMaps.get(e.getActionCommand()));

It doesn't make the code harder to read. If you prefer local variables, give them more meaningful names: `val` should rather be `lafClass` and `key` is `lafName` — that's what's stored in the map.

test/jdk/javax/swing/JRadioButton/bug4380543.java line 160:

> 158:         SwingUtilities.updateComponentTreeUI(this);
> 159:     }
> 160: }

Usually, there should be a new line character in the end of the file.

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

Changes requested by aivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8721