<Swing Dev> [OpenJDK 2D-Dev] [15] Review Request: 8237746 Fixing compiler warnings in src/demo/share/jfc
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Wed Mar 11 00:12:59 UTC 2020
I plan to push this change if there are no more comments about the webrev.
On 3/2/20 10:31 am, Alexander Zuev wrote:
> Looks much better. I would double the proposal of creating the separate issue for bringing formatting in different files to the same coding standard. Seeing different spacing on cycles and method invocations in different classes makes me cringe. But functionally it seems everything is fine.
>
> /Alex
>
> On 22-Feb-20 12:50, Sergey Bylokhov wrote:
>> 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