<Swing Dev> [15] Review Request: 8237746 Fixing compiler warnings in src/demo/share/jfc
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Sat Feb 22 09:50:20 UTC 2020
Thank you, an updated version is upload:
http://cr.openjdk.java.net/~serb/8237746/webrev.01
On 2/17/20 11:55 am, Marc Hoffmann wrote:
> 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
>
>
>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list