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.iva...@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.bylok...@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
JDK8237746-review.01.patch
Description: Binary data
