Simple JavaFX app pops-up reminders on schedule

Question

My employer gave me a sit-stand desk, but I was forgetting to use it: I just sat almost all day. So for fun and learning I wrote a simple JavaFX reminder app to remind me to alternate between sitting and standing. The app is pictured below:

In the screenshot the app is telling me to stand, and it shows the time the reminder was posted. When a hardcoded amount of time passes (25 minutes), the app window comes to the foreground and displays the next reminder. The next reminder is automatically scheduled.

The "Switch" button skips to the next reminder. I added it so that I could skip reminders that I didn't feel like doing. The app runs and displays reminders whether or not you use the button. I should rename it to "Skip Reminder".

I have no experience with JavaFX (or Swing) and almost no experience with UIs in general except for JSF and a little web, which is why I'm asking for a code review. My code probably falls short of best practices and maybe there are some bugs.

The initial project structure was generated by Netbeans using "New > Maven > JavaFX Application".

Here is the code. The interesting code is in FXMLController.java.

Scene.fxml

<?xml version="1.0" encoding="UTF-8"?>

<?import java.lang.*?>
<?import java.util.*?>
<?import javafx.scene.*?>
<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>

<VBox id="AnchorPane" xmlns:fx="http://javafx.com/fxml" fx:controller="link.sharpe.reminders.FXMLController">
    <children>
        <Button text="Switch" onAction="#handleButtonAction" fx:id="button" />
        <Label fx:id="label" />
        <Label fx:id="time" />
    </children>
</VBox>

MainApp.java

package link.sharpe.reminders;

import javafx.application.Application;
import static javafx.application.Application.launch;
import javafx.fxml.FXMLLoader;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class MainApp extends Application {

    static Stage stage;

    @Override
    public void start(Stage stage) throws Exception {
        this.stage = stage;
        Parent root = FXMLLoader.load(getClass().getResource("/fxml/Scene.fxml"));

        Scene scene = new Scene(root);
        scene.getStylesheets().add("/styles/Styles.css");

        stage.setTitle("Reminders");
        stage.setScene(scene);
        stage.show();
    }
}

FXMLController.java

package link.sharpe.reminders;

import java.net.URL;
import java.time.LocalTime;
import java.time.format.DateTimeFormatter;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.ResourceBundle;
import javafx.animation.KeyFrame;
import javafx.animation.KeyValue;
import javafx.animation.Timeline;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Label;
import javafx.util.Duration;

public class FXMLController implements Initializable {

    @FXML
    private Label label;

    @FXML
    private Label time;

    private List<Reminder> reminders = Arrays.asList(
            new Reminder("stand", 25),
            new Reminder("short break", 5),
            new Reminder("sit", 25),
            new Reminder("long break", 15));
    private Iterator<Reminder> reminderIterator = reminders.iterator();

    private Timeline timeline;

    @FXML
    void handleButtonAction(ActionEvent event) {
        scheduleNextReminder();
    }

    @Override
    public void initialize(URL url, ResourceBundle rb) {
        scheduleNextReminder();
    }

    private KeyValue[] scheduleNextReminder() {
        if (timeline != null) {
            timeline.stop();
            timeline = null;
        }
        if (!reminderIterator.hasNext()) {
            reminderIterator = reminders.iterator();
        }
        Reminder reminder = reminderIterator.next();

        label.setText(reminder.message);
        time.setText(LocalTime.now().format(DateTimeFormatter.ofPattern("h:mm a")));
        MainApp.stage.setIconified(false);
        MainApp.stage.toFront();

        timeline = new Timeline(new KeyFrame(
                Duration.minutes(reminder.duration),
                ae -> scheduleNextReminder()));
        timeline.play();
        return null;
    }

    private static class Reminder {

        String message;
        int duration;

        Reminder(String message, int duration) {
            this.message = message;
            this.duration = duration;
        }
    }

}

Styles.css

.button {
    -fx-font-weight: bold;
}

.label {
    -fx-font-size: 16pt;
}

(The whole project is hosted on this Git repo.)

Vogel612 · Accepted Answer · 2016-12-02 01:34:21Z

<VBox id="AnchorPane" ...

This is a little ... let's say problematic. The first problem is that you use a different type of component as the id. The second problem is the casing of the id. It's more conventional to give camelCase names. Then again this doesn't even need an id, because the ID you have is never used.

Similar considerations apply for the button "Switch".

import javafx.application.Application;
import static javafx.application.Application.launch;

You're mixing static imports with normal imports here. That's surprising. I'd expect imports to be ordered by package names after previously ordering whether they're static or not. Static imports are reserved for members of classes. That's why they're different from normal imports. Accordingly they should be someplace different than normal imports.

Additonally I don't see you using it. I assume you omitted main for brevity.

handleButtonAction is a rather bad name for a method. It'd be better off named onButtonClick, but even that is utterly nondescript. You can circumvent the signature mismatch of scheduleNextReminder() and an ActionEventListener in a lambda method. Consider using the following line in initialize:

button.setOnAction(evt -> scheduleNextReminder());

this does almost the same, but it circumvents the type signature mismatch that the FXMLLoader is probably complaining about.

private KeyValue[] scheduleNextReminder() {
    // lots of code
    return null;
}

wat? Just make that public void scheduleNextReminder(). Additional notes: Timeline is not the "proper" class to use when you want to Schedule something for later execution. What you're looking for is a ScheduledExecutorService. Instead of the little bit of mess you have right now, what you want is the following:

private void scheduleNextReminder() {
    if (currentReminder != null && !currentReminder.isDone) {
        currentReminder.cancel();
    }
    if (!reminderIterator.hasNext()) {
        reminderIterator = reminders.iterator();
    }
    Reminder reminder = reminderIterator.next();

    label.setText(reminder.message);
    time.setText(LocalTime.now().format(DateTimeFormatter.ofPattern("h:mm a")));
    MainApp.stage.toFront();

    currentReminder = executorService.schedule(this::scheduleNextReminder,
        reminder.duration,
        TimeUnit.MINUTES);
}

Do note that this is still a bit wonky, because you can just click the button to skip through reminders. Additionally you only need to set the stage's iconified property to false once at the start.

I also find the way the timestamp functionality is implemented rather lacking in the way that everything is inlined.

Last but not least I want to throw into the ring, that you only schedule a reminder when it's acknowledged by clicking the button. Waiting for that click to update the passed reminder details is a bit ineffective. You should consider updating the details and only doing the currentReminder related lines in scheduleNextReminder and extract the "presentation" code into a method like reminderElapsed(Reminder rem).

Thanks for the great review, Vogel. It turns out I was able to use onAction="#scheduleNextReminder" without issue: no need for button.setOnAction. — user10629
– user10629, Commented Dec 2, 2016 at 4:44
I used Timeline because of this blog post and because I kept reading that background threads shouldn't update UI compontents; the UI thread should update UI components. None of the articles really explained why. They just warned of "unpredictable" results. Some of the articles said to use Platform::runLater from the background thread. — user10629
– user10629, Commented Dec 2, 2016 at 4:50
"Do note that this is still a bit wonky, because you can just click the button to skip through reminders." I think my application description was a bit wonky because that's how I intended it to work: the "Switch" button is a "Skip" button, not an "Acknowledgement" button. I added it so that I could skip phases. When the timer fires, the next reminder is actually scheduled immediately, no need to press the button. My fault for making a misleading UI. Again, thanks for the great review. — user10629
– user10629, Commented Dec 2, 2016 at 4:59
I'm going to wait to see if I get more feedback for a day or two, and then I'll mark an answer correct. — user10629
– user10629, Commented Dec 2, 2016 at 18:10

Stack Exchange Network

Simple JavaFX app pops-up reminders on schedule

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Simple JavaFX app pops-up reminders on schedule

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions