RFR: 8287822: [macos] Remove support of duplicated formats from macOS [v2]
Alexander Matveev
almatvee at openjdk.org
Wed Oct 12 22:43:00 UTC 2022
On Tue, 11 Oct 2022 21:19:05 GMT, Kevin Rushforth <kcr at 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
More information about the openjfx-dev
mailing list