<Swing Dev> [15] Review Request: 8237746 Fixing compiler warnings in src/demo/share/jfc

[Marc Hoffmann]

Thanks Alexey for the detailed review! I attached a updated version.

The examples have many cleanup opportunities. I wanted to focus on compiler warnings for now and keep the changeset minimal.


> *Font2DTest.java*
> 674         if ( selectedText == fp.USER_TEXT )
> 675           userTextDialog.setVisible(true);
> 676         else
> 677           userTextDialog.setVisible(false);
> 
> I'd put the braces around and indent the statements by 4 spaces.
> However, it's the style used throughout the file: if there's only one statement, there are no braces and the statement is indented by 2 spaces rather than 4. Probably, to keep the code consistent, it's better to leave it as is.
> 
> 797             else
> 798               fontInfoDialog.setVisible(false);

Maybe separate issue for formatting?

> 
> *FontPanel.java*
> 1248            if (valArray == null) {
> 1249                valArray = EnumSet.allOf(FMValues.class).toArray(new FMValues[0]);
> 1250            }
> 1259            if (valArray == null) {
> 1260                valArray = EnumSet.allOf(FMValues.class).toArray(new FMValues[0]);
> 1261            }
> Can it be replaced with FMValues.values() as you did in Font2DTest.java lines 153, 156?
> 
> And below
> 1311                valArray = EnumSet.allOf(AAValues.class).toArray(new AAValues[0]);
> 1324                valArray = EnumSet.allOf(AAValues.class).toArray(new AAValues[0]);

Done.

> 
> *ButtonDemo.java*
> 64     Vector<Component> buttons = new Vector<>();
> Shall it be JComponent?

Doesn’t because JPanel.add() returns Component:

    buttons.add(p2.add(new JButton(getString("ButtonDemo.button1"))));

Should I introduce a local variable?

> 
> 
> *ComboBoxDemo.java*
> 60     JComboBox<?> hairCB;
> Why not JComboBox<String> ?
> All createXXX methods use this type.
> Then the cast below would be unnecessary:
> 282             String name = (String) parts.get(hairCB.getSelectedItem());

This comes from the lookup in the parts Hashtable. Unfortunately it has String an ImageIcon values.

> 
> 
> 114         presetCB = (JComboBox<?>) comboBoxPanel.add(createPresetComboBox());
> To avoid cast, you can use two statements:
> presetCB = createPresetComboBox();
> comboBoxPanel.add(presetCB);

Done for all 4 occurrences.

> 
> 
> *DirectionPanel.java*
> 97                 AbstractButton b = e.nextElement();
> 98             if( b.getActionCommand().equals(selection) ) {
> Indentation on line 97 seems incorrect, it should align to line 98, shouldn't it?

Done (replace tab with spaces).

> 
> 
> *SliderDemo.java*
> 167         @SuppressWarnings("unchecked")
> 168                 Dictionary<Object, Object> labelTable = s.getLabelTable();
> Would using Dictionary<?, ?> suppress the warning automatically?
> I mean that @SuppressWarning would become unnecessary.

Dictionary<?,?> does not allow put of specific types in the next line. But fixed tabs in the same line.

> 
> 
> *SplitPaneDemo.java*
> 168 divSize.setText(Integer.valueOf(splitPane.getDividerSize()).toString());
> can be simplified to
> divSize.setText(Integer.toString(splitPane.getDividerSize()));
> by using static method Integer.toString() method.

Done.

> 
> 
> Shall the copyright year be updated in all the modified files?

Please let me know what would be the correct process.

Cheers,
-marc




> On 17. Feb 2020, at 15:40, Alexey Ivanov <alexey.ivanov at oracle.com> wrote:
> 
> Thank you, Marc, for your contribution.
> And thank you to Sergey for creating the review.
> 
> *Font2DTest.java*
> 674         if ( selectedText == fp.USER_TEXT )
> 675           userTextDialog.setVisible(true);
> 676         else
> 677           userTextDialog.setVisible(false);
> 
> I'd put the braces around and indent the statements by 4 spaces.
> However, it's the style used throughout the file: if there's only one statement, there are no braces and the statement is indented by 2 spaces rather than 4. Probably, to keep the code consistent, it's better to leave it as is.
> 
> 797             else
> 798               fontInfoDialog.setVisible(false);
> 
> 
> *FontPanel.java*
> 1248            if (valArray == null) {
> 1249                valArray = EnumSet.allOf(FMValues.class).toArray(new FMValues[0]);
> 1250            }
> 1259            if (valArray == null) {
> 1260                valArray = EnumSet.allOf(FMValues.class).toArray(new FMValues[0]);
> 1261            }
> Can it be replaced with FMValues.values() as you did in Font2DTest.java lines 153, 156?
> 
> And below
> 1311                valArray = EnumSet.allOf(AAValues.class).toArray(new AAValues[0]);
> 1324                valArray = EnumSet.allOf(AAValues.class).toArray(new AAValues[0]);
> 
> 
> *ButtonDemo.java*
> 64     Vector<Component> buttons = new Vector<>();
> Shall it be JComponent?
> 
> 
> *ComboBoxDemo.java*
> 60     JComboBox<?> hairCB;
> Why not JComboBox<String> ?
> All createXXX methods use this type.
> Then the cast below would be unnecessary:
> 282             String name = (String) parts.get(hairCB.getSelectedItem());
> 
> 
> 114         presetCB = (JComboBox<?>) comboBoxPanel.add(createPresetComboBox());
> To avoid cast, you can use two statements:
> presetCB = createPresetComboBox();
> comboBoxPanel.add(presetCB);
> 
> 
> *DirectionPanel.java*
> 97                 AbstractButton b = e.nextElement();
> 98             if( b.getActionCommand().equals(selection) ) {
> Indentation on line 97 seems incorrect, it should align to line 98, shouldn't it?
> 
> 
> *SliderDemo.java*
> 167         @SuppressWarnings("unchecked")
> 168                 Dictionary<Object, Object> labelTable = s.getLabelTable();
> Would using Dictionary<?, ?> suppress the warning automatically?
> I mean that @SuppressWarning would become unnecessary.
> 
> 
> *SplitPaneDemo.java*
> 168 divSize.setText(Integer.valueOf(splitPane.getDividerSize()).toString());
> can be simplified to
> divSize.setText(Integer.toString(splitPane.getDividerSize()));
> by using static method Integer.toString() method.
> 
> 
> Shall the copyright year be updated in all the modified files?
> 
> 
> On 23/01/2020 08:54, Marc Hoffmann wrote:
>> Hi Sergey,
>> 
>> thanks for sponsoring this patch!
>> 
>> I successfully applied the webrev patch on current JDK head (57807:7bae17e00566). The build runs without warnings on the demo code :)
>> 
>> But I noticed a minor glitch: I inserted a tab in src/demo/share/jfc/SwingSet2/DirectionPanel.java Line 97 where only spaces are used in the rest of the file. Probably this should be fixed before merge.
>> 
>> Regards,
>> -marc
>> 
>> 
>>> On 23. Jan 2020, at 01:35, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> wrote:
>>> 
>>> Hello.
>>> Please review the fix for compiler warnings in the demo/jfc in JDK 15.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8237746
>>> Fix: http://cr.openjdk.java.net/~serb/8237746/webrev.00
>>> 
>>> This fix contributed by the Marc Hoffmann:
>>> https://mail.openjdk.java.net/pipermail/swing-dev/2019-October/009846.html
>>> 
>>> Fixed warnings: raw types, deprecated APIs, deprecated Applet APIs.
>>> 
>>> -- 
>>> Best regards, Sergey.
>> 
> -- 
> Regards,
> Alexey

-------------- next part --------------
A non-text attachment was scrubbed...
Name: JDK8237746-review.01.patch
Type: application/octet-stream
Size: 91426 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/swing-dev/attachments/20200217/8aa84bb9/JDK8237746-review.01-0001.patch>
-------------- next part --------------