On Sun, 17 Jul 2022 14:11:22 GMT, Nir Lisker <[email protected]> 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
