<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