Media player component with JavaFX

Question

I am trying to build a media player as a part of a bigger project in JavaFX. It is a simple media player as of now, which requires a Presentation component to output videos to. The dependencies are being injected by Guice.

This is my first time working on a Java project, so I would love some reviews on my code, especially based on design patterns, SOLID/DRY principles, code readability/re-usability/maintainability and how I can improve.

/**
 * Media Player component for Quizatron
 *
 * @author Dedipyaman Das <[email protected]>
 * @version 1.0.18.1
 * @since 1.0.18.1
 */
public class Player {
 private MediaView mediaView;
 private MediaPlayer mediaPlayer;
 private FileChooser fileChooser;
 private Presentation presentation;
 @FXML
 private AnchorPane playerNode;
 @FXML
 private Button playBtn;
 @FXML
 private Label currTimeLbl;
 @FXML
 private Label endTimeLbl;
 @FXML
 private JFXSlider timeSlider;
 @FXML
 private Label mediaInfo;
 @FXML
 private Label sourceFileLbl;
 /**
 * Media player component constructor.
 * Guice or any other injector needs to inject a {@link FileChooser} for getting the media file,
 * {@link MediaView} for visual output
 * and {@link Presentation} to have it presented in a separate view
 *
 * @param fileChooser FileChooser - single file
 * @param mediaView MediaView - Parent container for visual output
 * @param presentation Presentation state for separate stage
 */
 @Inject
 public Player(FileChooser fileChooser, MediaView mediaView, Presentation presentation) {
 this.mediaView = mediaView;
 this.fileChooser = fileChooser;
 this.presentation = presentation;
 }
 public enum PlayerState {
 PLAY, PAUSE
 }
 /**
 * Initializer to get the FXML components working.
 */
 @FXML
 public void initialize() {
 timeSlider.setValue(0);
 prepareMediaView();
 }
 /**
 * Prepare the mediaview window with correct dimensions
 * Binds the mediaview's width and height relative to the window size and video ratio
 *
 * @see MediaPresentationView
 * @see Bindings
 */
 private void prepareMediaView() {
 DoubleProperty width = mediaView.fitWidthProperty();
 DoubleProperty height = mediaView.fitHeightProperty();
 width.bind(Bindings.selectDouble(mediaView.sceneProperty(), "width"));
 height.bind(Bindings.selectDouble(mediaView.sceneProperty(), "height"));
 }
 /**
 * Open the file chooser and autoplay the media
 *
 * @param event ActionEvent
 * @throws Exception thrown on failure to open file
 */
 @FXML
 private void chooseMediaFromFile(ActionEvent event) throws Exception {
 fileChooser.setTitle("Open Resource File");
 File file = fileChooser.showOpenDialog(playerNode.getScene().getWindow());
 String source = file.toURI().toURL().toExternalForm();
 Media media = new Media(source);
 startPlayer(media);
 }
 /**
 * Loads and plays the media source
 *
 * @param media Media - source media to be played
 */
 private void startPlayer(Media media) {
 loadMedia(media);
 timeSlider.setValue(this.mediaPlayer.getCurrentTime().toSeconds());
 mediaPlayer.setOnReady(this::displayMetaData);
 initTimeSlider();
 initUIControlsBehavior();
 playLoadedMedia();
 }
 /**
 * Loads the media and instantiates a new media player
 *
 * @param media Media - reusable media component, can be used from anywhere
 */
 public void loadMedia(Media media) {
 if (mediaPlayer != null) {
 mediaPlayer.dispose();
 }
 this.setMediaPlayer(new MediaPlayer(media));
 }
 /**
 * Fetches and substitutes the placeholders for media metadata
 */
 private void displayMetaData() {
 timeSlider.setMax(mediaPlayer.getTotalDuration().toSeconds());
 ObservableMap<String, Object> metaData = mediaPlayer.getMedia().getMetadata();
 String artistName = (String) metaData.get("artist");
 String title = (String) metaData.get("title");
 String album = (String) metaData.get("album");
 String mediaSource = mediaPlayer.getMedia().getSource();
 mediaInfo.setText(title + " - " + artistName + " - " + album);
 sourceFileLbl.setText(getFileNameFromPath(mediaSource));
 double duration = mediaPlayer.getTotalDuration().toSeconds();
 endTimeLbl.setText(formatTime(duration));
 }
 /**
 * Get the slider running and enable seeking
 * {@link JFXSlider}
 */
 private void initTimeSlider() {
 timeSlider
 .valueProperty()
 .addListener((observable, oldValue, newValue)
 -> sliderSeekBehavior(oldValue, newValue));
 mediaPlayer
 .currentTimeProperty()
 .addListener((observable, oldDuration, newDuration)
 -> sliderProgressBehavior(oldDuration, newDuration));
 initIndicatorValueProperty();
 }
 /**
 * The slider behavior on dragging/clicking on the JFXSlider to seek
 *
 * @param oldValue Number - before seeking
 * @param newValue Number - after action on slider
 */
 private void sliderSeekBehavior(Number oldValue, Number newValue) {
 // Is the change significant enough?
 // Drag was buggy, have to run some tests
 // Affects only the drag it seems
 double tolerance = 1;
 if (mediaPlayer.getTotalDuration().toSeconds() <= 100) {
 tolerance = 0.5;
 }
 if (abs(oldValue.doubleValue() - newValue.doubleValue()) >= tolerance) {
 mediaPlayer.seek(Duration.seconds(newValue.doubleValue()));
 }
 }
 /**
 * Behavior of the slider as it progresses
 *
 * @param oldDuration
 * @param newDuration
 */
 private void sliderProgressBehavior(Duration oldDuration, Duration newDuration) {
 // Making sure it doesn't interfere with the manual seeking
 double newElapsedTime = newDuration.toSeconds();
 double oldElapsedTime = oldDuration.toSeconds();
 double totalDuration = mediaPlayer.getTotalDuration().toSeconds();
 if (!timeSlider.isValueChanging()) {
 if (newElapsedTime - oldElapsedTime >= 0.1) {
 timeSlider.setValue(newElapsedTime);
 }
 updateTimeLabel(totalDuration, newElapsedTime);
 }
 }
 /**
 * Setting the time elapsed and time left on the appropriate indicators
 */
 private void updateTimeLabel(double totalDuration, double elapsedTime) {
 // Get rid of the unnecessary decimal points
 double timeLeft = totalDuration - elapsedTime;
 String elapsedTimeFormatted = formatTime(elapsedTime);
 String remainingTimeFormatted = formatTime(timeLeft);
 // Time elapsed/left indicators update
 currTimeLbl.setText(elapsedTimeFormatted);
 endTimeLbl.setText(remainingTimeFormatted);
 }
 /**
 * Display indicator in HH:MM format
 */
 private void initIndicatorValueProperty() {
 // Only the guy on StackOverflow and god knows how this works
 timeSlider.setValueFactory(new Callback<JFXSlider, StringBinding>() {
 @Override
 public StringBinding call(JFXSlider arg0) {
 return Bindings.createStringBinding(new java.util.concurrent.Callable<String>() {
 @Override
 public String call() {
 return formatTime(timeSlider.getValue());
 }
 }, timeSlider.valueProperty());
 }
 });
 }
 /**
 * Sets the behavior of the player UI components based on the player state
 */
 private void initUIControlsBehavior() {
 mediaPlayer.setOnEndOfMedia(this::stop);
 // Multiline lambdas should be avoided? - Venkat S.
 mediaPlayer.setOnStopped(() -> {
 // Needs to be revisited
 togglePlayPauseBtn(PlayerState.PLAY);
 timeSlider.setValue(0);
 });
 mediaPlayer.setOnPaused(() -> togglePlayPauseBtn(PlayerState.PLAY));
 mediaPlayer.setOnPlaying(() -> togglePlayPauseBtn(PlayerState.PAUSE));
 }
 /**
 * Change the pause button to a startPlayer button and have the appropriate action based on it
 *
 * @param state current state of the player
 * {@link FontAwesomeIconView}
 */
 private void togglePlayPauseBtn(PlayerState state) {
 FontAwesomeIconView icon;
 if (state.equals(PlayerState.PLAY)) {
 icon = getIcon(FontAwesomeIcon.PLAY);
 playBtn.setOnAction(this::play);
 } else {
 icon = getIcon(FontAwesomeIcon.PAUSE);
 playBtn.setOnAction(this::pause);
 }
 playBtn.setGraphic(icon);
 }
 /**
 * Check if the media player was already there, prepare the presentation and startPlayer the video
 */
 private void playLoadedMedia() {
 if (mediaView.getMediaPlayer() != this.mediaPlayer) {
 preparePresentation();
 }
 mediaPlayer.play();
 }
 /**
 * Prepare the external presentation view
 * Get the view controller and embed visual output
 * {@link Presentation}
 */
 private void preparePresentation() {
 mediaView.setMediaPlayer(this.mediaPlayer);
 try {
 if (!(presentation.getView() instanceof MediaPresentationView)) {
 presentation.changeView("media-view");
 }
 MediaPresentationView mediaViewController = (MediaPresentationView) presentation.getView();
 mediaViewController.embedMediaView(mediaView);
 }
 catch (Exception e) {
 e.printStackTrace();
 }
 }
 /**
 * User action to startPlayer the media
 *
 * @param event ActionEvent
 */
 @FXML
 private void play(ActionEvent event) {
 mediaPlayer.play();
 }
 /**
 * User action event to pause the video
 *
 * @param event
 */
 @FXML
 private void pause(ActionEvent event) {
 mediaPlayer.pause();
 }
 /**
 * User action event to stop the media
 */
 public void stop() {
 mediaPlayer.stop();
 }
 public void openPlaylist() {
 }
 public void back() {
 }
 public void next() {
 }
 // Helper functions
 /**
 * Setter for the class media player
 *
 * @param mediaPlayer MediaPlayer {@link MediaPlayer}
 */
 private void setMediaPlayer(MediaPlayer mediaPlayer) {
 this.mediaPlayer = mediaPlayer;
 }
 /**
 * Extracts the filename + extension from the supplied file path
 *
 * @param filePath full file path
 * @return the filename stripped of slashes and everything before
 */
 private String getFileNameFromPath(String filePath) {
 filePath = filePath.replace("%20", " ");
 return filePath.substring(filePath.lastIndexOf('/') + 1, filePath.length());
 }
 /**
 * Creates a FontAwesomeIconView based on the supplied icon type
 *
 * @param iconType FontAwesome icon to be built
 * @return The final icon with appropriate styles and glyph sizes
 */
 private FontAwesomeIconView getIcon(FontAwesomeIcon iconType) {
 FontAwesomeIconView icon = new FontAwesomeIconView(iconType);
 icon.setGlyphSize(16);
 icon.setStyleClass("trackBtnIcon");
 return icon;
 }
 /**
 * Formats the time to a MM:SS format as a string
 *
 * @param totalSeconds the time specified in seconds
 * @return the formatted time string
 */
 private String formatTime(double totalSeconds) {
 int min = (int) totalSeconds / 60;
 int sec = (int) totalSeconds % 60;
 return String.format("%02d:%02d", min, sec);
 }
}

Any criticism is welcome, thanks for helping me improve my code!

Update: I have made some changes to the original code after having suggestions from people, since there are no answers yet- I would still like to have it reviewed. The purpose of the class remains the same.

Answer 1

I've never used fxml or jfoenix, so I may be missing some issues specific to those libraries, but I did notice a couple of points you could think about:

1. The logic in getFileNameFromPath() will break if you get anything fancier than a space in your filenames: you might get other URL-encoded characters, so better to use URLDecoder for that, and then use File.getName() to extract the filename:

   String cleanPath = java.net.URLDecoder.decode(filePath,"UTF-8");
   return java.nio.file.Paths.get(cleanPath).toFile().getName();
   

2. Your formatTime() method will truncate times over 99 mins, so depending on how long your files play for, you might want to change to String.format("%03d:%02d", min, sec) so you can go up to 999 minutes.

3. In chooseMediaFromFile() there's the possibility that the User can make no selection, in which case fileChooser.showOpenDialog() will return null, and file.toURI() will throw a NPE error: you might want to add a null check in order to handle that more gracefully.

4. Also, it's entirely optional, but your last 3 helper functions could be static if you like. I find that makes it clearer that they have no dependency on the rest of your class, and makes things a bit easier if you want to write unit tests.

---

I can also try to explain what's happening in initIndicatorValueProperty():

Basically you're calling a method on timeSlider and providing an argument in the form of an anonymous Callback class which contains a second anonymous Callable class.

1. Outside: the timeSlider.setValueFactory() method takes a Callback<JFXSlider, StringBinding> argument: that means any object (or lambda expression) that implements a call() function that accepts a JFXSlider object and returns a StringBinding. The purpose of the callback is to let the timeSlider answer the question "given the state I'm in, what text should I display?".

So the first @Override is an implementation of Callback.call(), which generates a StringBinding using Bindings.createStringBinding().

2. Inside: createStringBinding() takes a Callable<String> argument, which means any object or lambda that implements a call() function (no arguments) that returns a String.

So the second @Override is an implemetation of Callable.call(), which does the actual work: it takes the current value of timeSlider.getValue() in seconds, and uses your formatTime() method to turn it into a String in the form mm:ss.

Finally you have timeSlider.valueProperty() as the second argument to the outer call.

Using lambdas, you could re-write that entire bit of code as

timeSlider.setValueFactory( slider -> 
 Bindings.createStringBinding( 
 () -> formatTime( slider.getValue() ),
 slider.valueProperty()
 )
);

Note that the slider variable is just a placeholder for the input to the first call() method: that input will be the timeSlider object itself. In your original code you have arg0 but then you ignore that and re-use your timeSlider class variable which will work but it's a bit less clear..

If you look in the comments in the sourcecode for JFXSlider you can see a similar example, also in the form of a lambda expression:

// For example, to have the value displayed as a percentage
// (assuming the slider has a range of (0, 100)):
mySlider.setValueFactory(slider ->
 Bindings.createStringBinding(
 () -> ((int) slider.getValue()) + "%",
 slider.valueProperty()
 )
);

Hope that helps!

• twodee: Thank you for your review, and taking some of your extra time to address my understanding of the initIndicatorValueProperty()- which I did understand at a higher level, but failed to understand the lower levels of it. I would also appreciate a comment on how the code looks in terms of cleanliness, and ease to understand. The java.nio.file.Paths is also very helpful, I was looking for a similar solution to store paths. :D
• Richard Inglis: It's pretty easy to follow: I think you've done a good job overall :)
• twodee: Thank you very much for the thorough review! I am taking in your suggestions and testing it right away. I haven't worked with java.nio.file.Paths before, could you tell me if it is directly replaceable with the code I have in getFileNameFromPath method?
• Richard Inglis: Yes, you can replace the body of getFileNameFromPath() with the 2 lines I showed above.