Java Swing multithread management with observer pattern

Question

My goal is to draw a rectangle and move it smoothly from left to right, using Observer pattern.

I have a Model class, which is the Observable where I put the coordinates of the rectangle, and a Display class which is the Observer and perform repaint each time the coordinates change in the Model.

The coordinates changes in the Model are made in a while loop inside a SwingWorker : at each iteration I increment the x coordinate by one, then sleep for 100 ms, then notify the Observer (the Display) which only task is to perform a repaint. As you see the repaint() method is called on the EDT like it is adviced to do.

The problem is that the move isn't smooth after about one second, the repaint frequency change and it seems that the rectangle is less and less repainted.

Here is the Model class :

import java.util.Observable;
import java.awt.EventQueue;
import javax.swing.SwingWorker;

public class Model extends Observable{
    int xCoordinate;

    Model(Display d){
        SwingWorker<Void,Void> sw = new SwingWorker<Void,Void>(){

            @Override
            protected Void doInBackground() {
                while(xCoordinate<600){
                    xCoordinate ++;

                    try {
                        Thread.sleep(100);
                    } catch (InterruptedException ex) {}
                    setChanged();
                    notifyObservers(xCoordinate);
                }
                return null;
            }
        };
        addObserver(d);
        sw.execute();
    }

    public static void main(String[] a){
        EventQueue.invokeLater(new Runnable(){
            @Override
            public void run(){
                Display d = new Display();
                Model m = new Model(d);
                d.model = m;
            }
        });
    }
}

And here is the Display class :

import java.awt.Color;
import java.awt.EventQueue;
import java.awt.Graphics;
import java.util.Observable;
import java.util.Observer;
import javax.swing.JFrame;
import javax.swing.JPanel;

public class Display extends JFrame implements Observer{
    Model model;
    int xCoordinate;

    Display(){
        getContentPane().add(new JPanel(){
            @Override
            public void paintComponent(Graphics g){
                super.paintComponent(g);
                g.setColor(Color.RED);
                g.fillRect(xCoordinate, 1, 50, 50);
            }
        });
        setSize(600, 600);
        setVisible(true);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    @Override
    /* arg is the updated xCoordinate*/
    public void update(Observable o, Object arg) {
        xCoordinate = (Integer)arg;
        EventQueue.invokeLater(new Runnable(){
            @Override
            public void run() {
                repaint();
            }

        });
    }
}

I tried other methods, for instance using Timer in the Display, but that didn't work either. The SwingWorker maybe isn't useful here because calculations made on the SwingWorker thread are easy (increment by one) but I will need it for the heavy calculations I intend to do on my project (a pool game).

I also tried debugging by looking at time between two repaints (in Display) and time between two incrementations (in Model) and it was as expected about 100 ms each.

Thanks in advance

Answer 1

Okay, so as an initial test, I started with a Swing Timer...

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.util.Observable;
import java.util.Observer;
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();
                }

                Model model = new Model();

                JFrame frame = new JFrame("Testing");
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.add(new TestPane(model));
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setVisible(true);
                Timer timer = new Timer(40, new ActionListener() {
                                                            @Override
                                                            public void actionPerformed(ActionEvent e) {
                                                                model.update();
                                                            }
                                                        });
                timer.setInitialDelay(1000);
                timer.start();
            }

        });
    }

    public class Model extends Observable {

        private int xCoordinate;

        public void update() {
            xCoordinate++;
            setChanged();
            notifyObservers(xCoordinate);
        }

        public int getXCoordinate() {
            return xCoordinate;
        }

    }

    public class TestPane extends JPanel implements Observer {

        private Model model;

        public TestPane(Model model) {
            this.model = model;
            model.addObserver(this);
        }

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

        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            Graphics2D g2d = (Graphics2D) g.create();
            g.setColor(Color.RED);
            g.fillRect(model.getXCoordinate(), 1, 50, 50);
            g2d.dispose();
        }

        @Override
        public void update(Observable o, Object arg) {
            System.out.println(arg);
            repaint();
        }

    }

}

The thing I found was, you never call setChanged on the Observable, which during my testing, meant that it never called the Observers

I also did a test with a SwingWorker...

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.util.Observable;
import java.util.Observer;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.SwingWorker;
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();
                }

                Model model = new Model();

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

                SwingWorker worker = new SwingWorker() {
                    @Override
                    protected Object doInBackground() throws Exception {
                        Thread.sleep(1000);
                        while (true) {
                            model.update();
                        }
                    }
                };
                worker.execute();
            }

        });
    }

    public class Model extends Observable {

        private int xCoordinate;

        public synchronized void update() {
            xCoordinate++;
            setChanged();
            notifyObservers(xCoordinate);
        }

        public synchronized int getXCoordinate() {
            return xCoordinate;
        }

    }

    public class TestPane extends JPanel implements Observer {

        private Model model;

        public TestPane(Model model) {
            this.model = model;
            model.addObserver(this);
        }

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

        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            Graphics2D g2d = (Graphics2D) g.create();
            g.setColor(Color.RED);
            g.fillRect(model.getXCoordinate(), 1, 50, 50);
            g2d.dispose();
        }

        @Override
        public void update(Observable o, Object arg) {
            System.out.println(arg);
            repaint();
        }

    }

}

Because of the thread synchronization issues, I synchronized access to the methods to ensure that values weren't changing between updates. Because you're using an Observer, could actually pass the "new state" to the Observer, so they aren't reliant on then value of the model at the time they use them.

Okay, so the long and short of it, you need to call setChanged on the Observable once it's been updated, so that notifyObservers will actually call the Observers

As someone undoubtedly point out, this approach suffers from inaccuracies in the timing, by it's nature, both Swing Timer and Thread.sleep only guarantee a "at least" timing and may verify between each update.

If you have a variable length operation, this will also affect the time between updates. Instead, you should be calculating the time it took you to perform your operation, subtract the amount of time you want to wait, you would then use this delay to calculate how long you want to "wait" between frames. You should also be using System.nanoTime over System.currentTimeMillis as it won't suffer from the same system clock synchronization issues