[OpenJDK 2D-Dev] <Swing Dev> [15] Review Request: 8237746 Fixing compiler warnings in src/demo/share/jfc
Alexander Zuev
alexander.zuev at oracle.com
Mon Mar 2 18:31:36 UTC 2020
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
>>
>>
>>
>>
>
>
More information about the 2d-dev
mailing list