On Sun, 17 Jul 2022 14:11:22 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Fixes "Unlikely argument type" warning generated by the latest Eclipse IDE. >> >> This warning should be reclassified as an error, as it catches bugs missed >> by javac. In this case, the following places seem to contain bugs: >> - >> apps/samples/3DViewer/src/main/java/com/javafx/experiments/jfx3dviewer/TimelineController.java >> - >> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/GetEvent.java >> >> The fixes include: >> - using objects of the right type >> - adding @SuppressWarnings("unlikely-arg-type") in places where intended >> (e.g. assertFalse) >> >> There was an earlier discussion about using IDE-specific @SuppressWarnings >> in the code. While I might disagree (I think it is ok to use IDE-specific >> @SuppressWarnings), the tests can be reworked to avoid @SuppressWarnings. >> Please let me know. Thanks! > > apps/samples/3DViewer/src/main/java/com/javafx/experiments/jfx3dviewer/TimelineController.java > line 79: > >> 77: loopBtn.setDisable(false); >> 78: playBtn.setSelected(t.getCurrentRate() != 0); >> 79: >> loopBtn.setSelected(t.getCycleDuration().equals(Duration.INDEFINITE)); > > The correct fix is to check `t.getCycleCount() == Timeline.INDEFINITE`. Yes, given what the button does, it should be testing cycleCount not cycleDuration. > modules/javafx.base/src/test/java/test/javafx/collections/ObservableSubListTest.java > line 161: > >> 159: @Test >> 160: public void testEqualsOnAnotherType() { >> 161: assertFalse(sublist.equals(Integer.valueOf(7))); > > I'm not sure I see the point of this test. It's testing the implementation of > the backing list, not the behavior of the sublist. Same for some other > methods in this test, though it's not related to this PR. Let's see what > Kevin thinks here. It might be worth a separate bug to look at this. > modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStyleMap_Test.java > line 83: > >> 81: private void checkFoundStyle(Property<?> property, >> Map<StyleableProperty<?>, List<Style>> map, List<Declaration> decls) { >> 82: >> 83: List<Style> styles = map.get((StyleableProperty<?>)property); > > If the `Property` must be a `StyleableProperty` then maybe the method > signature should be that and the cast should be the responsibility of the > callers. Since this is in a unit test, I don't have a strong opinion about this. Also, since this is in a test, an `assertTrue(property instanceof StyleableProperty)` might not be a bad idea. > modules/javafx.graphics/src/test/java/test/javafx/css/Node_cssStyleMap_Test.java > line 300: > >> 298: return false; >> 299: } >> 300: > > I don't mind removing this method since it's unused, but probably as part of > a "remove unused methods" PR. In general, I agree with this as a best practice. In this case, the unused method would need to be fixed up, so I can also see the argument for removing it as part of eliminating this warning. I don't mind either way. ------------- PR: https://git.openjdk.org/jfx/pull/823