[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: RFR: 8286620: Create regression test for verifying setMargin() of JRadioButton [v3]
From:       Alexey Ivanov <aivanov () openjdk ! java ! net>
Date:       2022-05-31 11:34:40
Message-ID: DpvmclypIcCDXli-qMUwP9HpCimQ8JoEbEJXs6d-Nv4=.0c99e50c-3c26-492a-95cc-aa9b5302e861 () github ! com
[Download RAW message or body]

On Fri, 27 May 2022 17:21:29 GMT, Tejesh R <duke@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


[prev in list] [next in list] [prev in thread] [next in thread]