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