<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