<Swing Dev> [OpenJDK 2D-Dev] [15] Review Request: 8237746 Fixing compiler warnings in src/demo/share/jfc
Alexey Ivanov
alexey.ivanov at oracle.com
Wed Mar 11 19:28:33 UTC 2020
Hi Sergey,
The updated webrev.01 looks good to me.
Please go ahead and push the changes.
I agree with Alexander, separate issues for formatting are better.
Regards,
Alexey
On 11/03/2020 00:12, Sergey Bylokhov wrote:
> 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
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
More information about the swing-dev
mailing list