Display moving blocks in a JFrame

Question

I have a class that creates a JFrame on which a simple game of Tetris will be played, I also have a class DrawSquare, which does exactly what you think it does, however when I initialise a new instance of the DrawSquare class and then try to draw that one and all the others to my JFrame things start to go wrong, the code is intended for one square to be drawn in the top left hand corner and then drop down a line at a time until it reaches the bottom of the frame (it does this), then a new square should be drawn in the second column at the top of the frame, as well as our first square in the bottom left hand corner, however once it starts dropping down the second column I get a series of squares drawn in a diagonal towards the top right hand corner. At the moment all I plan for the code to do is have a square drop from the top row of each column and stop when it reaches the bottom of the frame, am I storing the instance of the class at the wrong point in the code? Edit: In fact I'm pretty sure it's that, I'd want to store that instance when it reaches the bottom. Does every instance of the class need its own timer?

public class Tetris extends JFrame {
    public static final int height = 20; //height of a square
    public static final int width = 20; //width of a square
    public int xPos = 0; //column number of the square
    public int yPos = 0; //row number of the square

    public static void main(String[] args){
            Tetris tet = new Tetris();
    }

    public Tetris() {
        DrawSquare square = new DrawSquare(xPos, yPos, width, height, false);
        add(square);
        DrawSquare.squares.add(square);
        setSize(220,440);
        setLocationRelativeTo(null);
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setVisible(true);
    }
}


public class DrawSquare extends JPanel {
    public static List squares = new ArrayList<>();
    protected int xPos;
    protected int yPos;
    protected int width;
    protected int height;
    protected Timer timer = new Timer(200, new TimerListener());
    protected boolean endFall = false;


    public DrawSquare(int xPos, int yPos, int width, int height, boolean endFall) {
        this.xPos = xPos;
        this.yPos = yPos;
        this.width = width;
        this.height = height;
        this.endFall = endFall;
        this.timer.start();
    }

    class TimerListener implements ActionListener {
        @Override
        public void actionPerformed(ActionEvent e) {
            yPos++;
            if (yPos > 19) {
                yPos = 19;
                endFall = true;
            }
            if (endFall == true) {
                timer.stop();
                if (xPos > 8) {
                    xPos = 8;
                }
                xPos++;
                endFall = false;
                yPos = 0;
                DrawSquare newSqr = new DrawSquare(xPos, yPos, width, height, true);
                squares.add(newSqr);
                add(newSqr);
            }
            timer.start();
            repaint();
        }
    }

    protected void paintComponent(Graphics g) {
        super.paintComponent(g);
        Iterator it = squares.iterator();
        while (it.hasNext()) {
            DrawSquare square = it.next();
            g.fillRect(square.xPos * square.width, square.yPos * square.height, square.width, square.height);
        }
    }
}

Mad Physicist · Accepted Answer

You are giving a great example of the fundamental misunderstanding beginners have of how the swing (and many other graphics toolkits) render stuff to the screen. I will give an overview of that, as it pertains to you, then answer your immediate questions and explain how to fix your code.

It took me a (very long) while to figure out how this stuff works my self, so please bear with me. I hope that reading through this answer will help you in a much more general way than answering this one question.

Asynchronous Drawing

Swing draws windows in a totally different sequence (the event dispatching thread) than the ones that modifies the state of your program (the main thread, as well as timer and other threads). You can modify the coordinates of things you want to draw as many times as you like in the main thread, but the changes will not show up until you request them to by calling JComponent.repaint() on one of your components. This will generally trigger a nearly-immediate repaint of the component, displaying your latest state.

If you change the coordinates of a widget like a JPanel in your main thread, it will likely show up immediately. This is because the methods you use to set the position will trigger repaint requests internally.

A repaint request gets queued and eventually processed by the event dispatching thread. This is where the paintComponent method gets called. The paintComponent method should therefore only draw. It should not do any other logic. If it needs to know how to draw some specialized stuff, the information for that should be stashed somewhere accessible by one of the other threads.

In short, you make calculations and update state as you need in the main thread or the timer. Then you access that state in the event dispatching thread via the paintComponent method.

Timers

There are a bunch of ways you can use timers to run your GUI, but you only really need one for the current application. In your case, the timer only needs to do two things:

Check if a block has fallen all the way down and doesn't need to move any more.
Trigger a repaint of your panel.

You do not need to compute the updated position of the blocks in the timer if the block's position is a simple equation with respect to time. If you know the time at which a block appears on the screen and the current time, you know how far the block has moved, so you can paint it in the correct spot based purely on the elapsed time.

If you had a more complicated system with paths that you could not predict purely on the time, I would recommend sticking the movement logic into the timer events as well. In that case, you might consider having multiple timers, or switching to java.util.timer. But again, this does not apply to your current case (even with multiple blocks).

Model and View

The model of your program is the thing that holds the abstract state. In this case, the positions and other meta-data about all your blocks. The view is the part that does the rendering. It is usually a good idea to separate these two things. There is often a third component to GUIs, called the controller, which connects the model and view to the user. We will ignore it here since you are not asking about controlling the blocks yet.

In your current code, you have attempted to represent your blocks with an extension to JPanel and a static list of existing blocks. While a JPanel may be a convenient way to display rectangular blocks with some custom graphics in them (like icons), I would recommend that you start by drawing the blocks directly using the Graphics object passed to paintComponent. At least initially, it will help you to think of the drawing code and the game logic as separate entities.

Final Rant Before Code Dump

I have made rewrites to your code to encapsulate all the ranting I did before into code. Here are some additional minor points about what I did that may help explain my reasoning:

When you call JFrame.add(...) to add a component to a JFrame, you are really calling JFrame.getContentPane().add(...). The content pane is where 90% of normal swing components go in a window. Therefore, we can either set the JPanel that will do the rendering as your content pane or we can add it to the current content pane. I have chosen to do the latter so that you can add other widgets, like a score board, at a later time.
Class names should generally be nouns, while methods are often verbs. This is not an absolute rule (nothing really is), but naming things this way will often help you visualize the interactions between objects in a more meaningful way. I have renamed DrawSquare to GamePiece for this reason.
There is no longer any reason for GamePiece to be a JPanel. It just needs to know its own width, height, and time of appearance.
The other problem with trying to have DrawSquare draw itself is that a component can only really draw within its own bounding box. So you really want to override the paintComponent of whatever holds the rectangles.
The rendering class maintains a reference to two lists of GamePieces. One is for the moving objects and one is for the ones that have fallen. The logic for moving them between the lists is in the timer. This is better than say adding a flag to GamePiece because it facilitates incremental repaint. I will only partially illustrate this here, but there is a version of repaint that only requests a small region to be painted. This would be useful to speed up the movement.

Code

public class Tetris extends JFrame
{
    public static final int height = 20; //height of a square
    public static final int width = 20; //width of a square
    public static final int x = 0;

    private GamePanel gamePanel;

    public static void main(String[] args)
    {
        Tetris tet = new Tetris();
        // Normally you would tie this to a button or some other user-triggered action.
        tet.gamePanel.start();
        tet.gamePanel.addPiece(new GamePiece(width, height, x));
    }

    public Tetris()
    {
        getContentPane().setLayout(new BorderLayout());
        gamePanel = GamePanel();
        add(gamePanel, BorderLayout.CENTER);
        setSize(220,440);
        setLocationRelativeTo(null);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        setVisible(true);
    }
}

public class GamePanel extends JPanel
{
    private List moving;
    private List still;
    private Timer timer;

    public GamePanel()
    {
        moving = new ArrayList<>();
        still = new ArrayList<>();
        timer = new Timer(100, new TimerListener());
    }

    public addPiece(int width, int height, int x)
    {
        moving.add(new GamePiece(width, height, x));
    }

    public void start()
    {
        timer.start();
    }

    @Override
    public void paintComponent(Graphics g)
    {
        Rectangle clip = g.getClipBounds(null);
        Rectangle rectToDraw = new Rectangle();

        // I prefer this, but you can make the call every
        // time you call `GamePiece.getY()`
        long time = System.currentTimeMillis();

        for(GamePiece piece : this.moving) {
            rectToDraw.setSize(piece.width, piece.height)
            rectToDraw.setLocation(piece.x, piece.getY(time))
            if(rectangleToDraw.intersects(clip))
                ((Graphics2D)g).fill(rectToDraw)
        }

        for(GamePiece piece : this.still) {
            rectToDraw.setSize(piece.width, piece.height)
            rectToDraw.setLocation(piece.x, piece.getY(time))
            if(rectangleToDraw.intersects(clip))
                ((Graphics2D)g).fill(rectToDraw)
        }
    }

    private class TimerListener implements ActionListener
    {
        @Override
        public void actionPerformed(ActionEvent e)
        {
            long time = System.currentTimeMillis();
            // Using non-iterator loop to move the pieces that
            // stopped safely. Iterator would crash on in-loop move.
            for(int i = 0; i < moving.size(); i++) {
                piece = moving.get(i);
                if(piece.getY(time) > 440 - piece.height) {
                    moving.remove(i);
                    still.add(piece);
                    i--;
                }
            }
            repaint();
        }
    }
}

public class GamePiece
{
    public final int width;
    public final int height;
    public final long startTime;
    public int x;

    public GamePiece(int width, int height, int x)
    {
        this.width = width;
        this.height = height;
        this.startTime = System.currentTimeMillis();
        this.x = x;
    }

    public int getY(long time)
    {
        // This hard-codes a velocity of 10px/sec. You could
        // implement a more complex relationship with time here.
        return (int)((time - this.startTime) / 100.0);
    }
}