On Tue, 11 Oct 2022 21:19:05 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 73:
>>
>>> 71: if (javaEnv.clearException())
>>> 72: return NULL;
>>> 73:
>>
>> minor: would it make sense to wrap return in { }, line 67 and 72?
>
> A lot of our third-party native code doesn't (I would insist on the `{}` if
> this were Java code), so if this were a gstreamer file it would be better to
> follow their convention, but since this is entirely our code, it makes sense
> to enclose in `{}`.
Fixed.
>> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line
>> 66:
>>
>>> 64: * [Windows] jlink --output dist/FXMediaPlayer -p
>>> ../../../../build/jmods;dist --add-modules
>>> javafx.base,javafx.controls,javafx.media,javafx.graphics,FXMediaPlayer
>>> --launcher FXMediaPlayer=FXMediaPlayer/fxmediaplayer.FXMediaPlayer
>>> 65: * [Windows] dist\FXMediaPlayer\bin\FXMediaPlayer.bat
>>> 66: */
>>
>> minor: this javadoc will likely not render as expected. perhaps use <pre>
>> from line 50 to 61 or 65.
>> also, spelling "protcol" on line 50
>
> Also minor: you can shorten the command lines by removing `javafx.base` and
> `javafx.graphics` from the list of added modules, since `javafx.controls`
> transiently requires them.
Spelling fixed and javafx.base and javafx.graphics is removed. I do not think
that we need to worry about javadoc, since we do not generate it for this app.
>> tests/manual/media/FXMediaPlayer/src/fxmediaplayer/media/FXMedia.java line
>> 131:
>>
>>> 129: });
>>> 130: } catch (URISyntaxException | IOException ex) {
>>> 131: System.out.println("Exception: " + ex);
>>
>> should System.out be removed?
>>
>> general question: do we want to use logging in places like this?
>
> Since this is a test app, I wouldn't bother with logging. We generally would
> use `System.err` rather than `System.out` for printing exceptions, but that's
> a minor point.
I change it to System.err.
-------------
PR: https://git.openjdk.org/jfx/pull/909
