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.iva...@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.bylok...@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
