2

I am trying to implement a simple analogue clock in java. My clock successfully displays the time when I first run the program, however upon opening the time remains the same and none of the drawing updates and it freezes. I don't understand why it is not working and I have looked through the forum trying to find someone with a similar problem and cannot. If anyone can find where I am going wrong I would greatly appreciate it, thanks

import java.awt.BasicStroke;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.image.BufferedImage;
import java.util.Date;
import javax.swing.JFrame;
import javax.swing. JPanel;

public class ClockFace extends JPanel {

Date date = new Date();

private BufferedImage clockFace;

public ClockFace() {

    this.init();
    this.startClock();

}

private void init() {

    JFrame window = new JFrame("Clock");
    window.setContentPane(this);

    this.setPreferredSize(new Dimension(600,600));

    window.pack();
    window.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    window.setVisible(true);
    window.setLocationRelativeTo(null);

}

//Draws the image to the ClockFace panel
public void paintComponent(Graphics g) {

    Graphics2D twoD = (Graphics2D) g;
    twoD.drawImage(clockFace, 0, 0, null);

}

public void startClock() {

    while(true) {

        int seconds = date.getSeconds();
        int minutes = date.getMinutes();
        int hours = date.getHours();

        clockFace = new BufferedImage(600,600,BufferedImage.TYPE_INT_ARGB);
        Graphics2D twoD = clockFace.createGraphics();
        twoD.setColor(Color.BLACK);
        twoD.fillRect(0, 0, 600, 600);
        twoD.setColor(Color.WHITE);
        twoD.translate(300,300);

        //Drawing the hour markers
        for(int i=0; i<12; i++) {

            twoD.drawLine(0, -260, 0, -300);
            twoD.rotate(Math.PI/6);

        }

        twoD.rotate(seconds*Math.PI/30);
        twoD.drawLine(0, 0, 0, -290);

        twoD.rotate(2*Math.PI-seconds*Math.PI/30);
        twoD.rotate(minutes*Math.PI/30);
        twoD.setStroke(new BasicStroke(3));
        twoD.drawLine(0, 0, 0, -250);

        twoD.rotate(2*Math.PI-minutes*Math.PI/30);
        twoD.rotate(hours*Math.PI/6);
        twoD.setStroke(new BasicStroke(6));
        twoD.drawLine(0, 0, 0, -200);

        repaint();

        //Pausing until the next "tick"
        try {
            Thread.sleep(1000);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

    }

}

}
1
  • 1
    The get methods of Date are deprecated. Instead I recommend you use LocalTime, one of the classes from java.time, the modern Java date and time API. Also, are you ever changing the value of date? Link: Oracle Tutorial: Date Time.
    – Ole V.V.
    Apr 9, 2018 at 20:22

2 Answers 2

6

You're blocking the Event Dispatching Thread, preventing it from processing any new input events or processing any new paint events

See Concurrency in Swing for more details.

So instead of...

public void startClock() {
    while(true) {
       //...
    }
}

You should be using something more like...

public void startClock() {
    Timer timer = new Timer(500, new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent evt) {
            //... Update time and schedule repaint
        }
    }
}

See How to Use Swing Timers for more details

You will also find that this...

//Draws the image to the ClockFace panel
public void paintComponent(Graphics g) {

    Graphics2D twoD = (Graphics2D) g;
    twoD.drawImage(clockFace, 0, 0, null);

}

Gives you issues (paint artefacts). It should be..

//Draws the image to the ClockFace panel
@Override
protected void paintComponent(Graphics g) {
    super.paintComponent(g);
    Graphics2D twoD = (Graphics2D) g;
    twoD.drawImage(clockFace, 0, 0, null);

}

Updated...

There's been (too) much discussion over weather the core issue is to do with the potential blocking of the Event Dispatching Thread or the non-mutable nature of the Date class.

The fact is, is probably both.

java.util.Date is a snapshot of a point in time (when it's created), it doesn't change and is not updated. So, based on your example, you'd have to update the date instance on each iteration of the loop.

A better solution would probably be to use java.time.LocalTime, apart from been more up-to-date, it's generally simpler to use (IMHO)

For example...

import java.awt.BasicStroke;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.image.BufferedImage;
import java.time.LocalTime;
import java.time.temporal.TemporalField;
import java.util.Date;
import java.util.concurrent.TimeUnit;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.Timer;
import javax.swing.UIManager;
import javax.swing.UnsupportedLookAndFeelException;

public class Test {

    public static void main(String[] args) {
        new Test();
    }

    public Test() {
        EventQueue.invokeLater(new Runnable() {
            @Override
            public void run() {
                try {
                    UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
                } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | UnsupportedLookAndFeelException ex) {
                    ex.printStackTrace();
                }

                JFrame frame = new JFrame("Testing");
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.add(new ClockFace());
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setVisible(true);
            }
        });
    }

    public class ClockFace extends JPanel {

        public ClockFace() {
            this.startClock();
        }

        @Override
        public Dimension getPreferredSize() {
            return new Dimension(600, 600);
        }

        @Override
        protected void paintComponent(Graphics g) {
            Graphics2D g2d = (Graphics2D) g.create();

            LocalTime now = LocalTime.now();
            int seconds = now.getSecond();
            int minutes = now.getMinute();
            int hours = now.getHour();

            g2d.setColor(Color.BLACK);
            g2d.fillRect(0, 0, 600, 600);
            g2d.setColor(Color.WHITE);
            g2d.translate(300, 300);

            //Drawing the hour markers
            for (int i = 0; i < 12; i++) {

                g2d.drawLine(0, -260, 0, -300);
                g2d.rotate(Math.PI / 6);

            }

            g2d.rotate(seconds * Math.PI / 30);
            g2d.drawLine(0, 0, 0, -290);

            g2d.rotate(2 * Math.PI - seconds * Math.PI / 30);
            g2d.rotate(minutes * Math.PI / 30);
            g2d.setStroke(new BasicStroke(3));
            g2d.drawLine(0, 0, 0, -250);

            g2d.rotate(2 * Math.PI - minutes * Math.PI / 30);
            g2d.rotate(hours * Math.PI / 6);
            g2d.setStroke(new BasicStroke(6));
            g2d.drawLine(0, 0, 0, -200);

            g2d.dispose();
        }

        public void startClock() {
            Timer timer = new Timer(500, new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {
                    repaint();
                }
            });
            timer.start();
        }

    }

}
8
  • Using javax.swing.Timer is the best choice for this situation. However in general when you want to do something that takes a long time (and might block the EDT) look at SwingWorker, which is a general case class for background tasks less specialized than Timer. docs.oracle.com/javase/tutorial/uiswing/concurrency/worker.html
    – markspace
    Apr 9, 2018 at 19:46
  • 1
    He isn't actually blocking the EDT; the sleep provides enough time to let it run. But that's not the way to do it.
    – laune
    Apr 9, 2018 at 19:47
  • @markspace Just remember, SwingWorker has an additional overhead of synchronising (and because of the implementation places greater weight to the background thread) is a lot less regular then something like Timer Apr 9, 2018 at 19:48
  • @laune His ClockFace class extends JPanel. If he isn't calling its ctor on the EDT, it's wrong. And the ctor calls the while loop, meaning it will block the EDT.
    – markspace
    Apr 9, 2018 at 19:49
  • 1
    @laune In this case, if the OP is executing the code from within the context of the EDT (and based on their description they are), then Thread.sleep will sleep the EDT - preventing it from processing any new paint methods - also, the while-loop will continue to prevent the EDT from ever been able to process the event queue, because the method never exists Apr 9, 2018 at 20:13
1

Insert the computation of date right after while(true).

That's not the way to write a program doing a task scheduled at time increments. It only fixes your immediate problem. See other answers.

7
  • 1
    90% sure this is the wrong answer. "Blocking the EDT" is the correct idea.
    – markspace
    Apr 9, 2018 at 19:40
  • Well, it runs since 9:30 p.m. on my system. The sleep unblocks. Not that I'd write it like that... Timer, and all that...
    – laune
    Apr 9, 2018 at 19:41
  • @markspace there is no EDT blocking here... startclock() is not called from the EDT but from the main thread. What is wrong is that the date is never recomputed... Apr 10, 2018 at 6:43
  • @laune be more precise about the computation of date, you should says that dates are immutable so OP needs to construct a new date object each time he needs to determine the current date/time. Apr 10, 2018 at 6:45
  • 1
    @Jean-BaptisteYunès I'd say it's cause by both :P Apr 10, 2018 at 7:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.