JPanel not updating on second mouse click

Question

After finishing and running a simple program that lets you change the color of a shape when it's clicked on:

    import java.awt.*;
import java.awt.event.*; 
import javax.swing.*;

class MyPanel extends JPanel{
    int x = 200,y = 200,r = 50;
    static Color co = Color.BLUE;
    static final JFrame frame = new JFrame();
    public static void main(String[] args){
        frame.setTitle("Color Change with Mouse Click");
        frame.setSize(500,500);
        MyPanel pane = new MyPanel();
        frame.add(pane);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        pane.setVisible(true);
        }

    public void Panel(){
        addMouseListener(new MouseAdapter(){
            public void mouseClicked(MouseEvent e){
                if(((e.getX() >= (x-r) && e.getX() < (x+r) && e.getY() >= (y-r) && e.getY() < (y+r))) & (co == Color.GREEN)){
                    co = Color.BLUE;
                    repaint();
                };
                if(((e.getX() >= (x-r) && e.getX() < (x+r) && e.getY() >= (y-r) && e.getY() < (y+r))) & (co == Color.BLUE)){
                    co = Color.GREEN;
                    repaint();
                };
            }
        });
    }

    Timer timer = new Timer(1, new ActionListener(){
        @Override
        public void actionPerformed(ActionEvent e){
            Panel();
        }
    });

    @Override
    public void paintComponent(Graphics g){
        super.paintComponent(g);
        timer.start();
        Panel();
        g.setColor(co);
        g.fillOval(x-r, y-r, 2*r, 2*r);
        repaint();
    }
}

I encountered a problem that I simply don't know how to fix. The JPanel never updates on the second mouse click, only on the first. I thought that adding the timer would take care of this, but apparently it didn't. Help is much appreciated.

edit: I changed my code per Aqua's suggestions:

import java.awt.*;
import java.awt.event.*; 
import javax.swing.*;

class MyPanel extends JPanel{
    int x = 200,y = 200,r = 50;
    static Color co = Color.BLUE;

    public static void main(String[] args){
        final JFrame frame = new JFrame();
        frame.setTitle("Color Change with Mouse Click");
        MyPanel pane = new MyPanel();
        frame.add(pane);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
        }

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

    public void panel(){
        addMouseListener(new MouseAdapter(){
            public void mouseClicked(MouseEvent e){
                if(((e.getX() >= (x-r) && e.getX() < (x+r) && e.getY() >= (y-r) && e.getY() < (y+r))) && (co == Color.GREEN)){
                    co = Color.BLUE;
                    repaint();
                }else if(((e.getX() >= (x-r) && e.getX() < (x+r) && e.getY() >= (y-r) && e.getY() < (y+r))) && (co == Color.BLUE)){
                    co = Color.GREEN;
                    repaint();
                }else{
                    repaint();
                };
            }
        });
    }
    public MyPanel(){
        Timer timer = new Timer(20, new ActionListener(){
            @Override
            public void actionPerformed(ActionEvent e){
                panel();
            }
        });
        timer.start();
    }
    @Override
    public void paintComponent(Graphics g){
        super.paintComponent(g);
        g.setColor(co);
        g.fillOval(x-r, y-r, 2*r, 2*r);
        repaint();
    }
}

And the color change works now, however the color change is a bit unpredictable. sometimes it has a delay, sometimes it doesn't, and sometimes it just outright won't change back. Is this a problem with my timer?

One problem I see is that you are doing a "bitwise and" in your if conditions instead of a boolean and. You should have `...&& (co == (COLOR...))` instead of `...& (co == COLOR...))` — gla3dr, Nov 13 '13 at 15:46

score 2 · Answer 1 · edited May 23 '17 at 12:19

Calling Panel() from paintComponent() triggers an endless loop that results in StackOverflowError exception. As repaint() eventually results in a call to paintComponent().

You should attach a mouse listener once in the initialization of a panel, not on every repaint. Same goes for the timer. But, it is not clear what is the intention of timer in this sample.

Some other problems with the posted code:

Call frame.setVisible(true); instead of pane.setVisible(true); As posted the code does not work. There is also no need to keep a static member JFrame frame in the panel. You can simply declare and use JFrame it in main().
paintComponent() is for painting only. Avoid complex logic in that method. Do not setup timers or call other initialization methods. You have little control when paintComponent is executed and it has to be fast for optimal drawing results. Go through Performing Custom Painting tutorials. Move out initialization of timer and mouse listener to panel's constructor.
Do not use frame.setSize(500,500);, instead override JPanel.getPreferredSize(). See Should I avoid the use of set(Preferred|Maximum|Minimum)Size methods in Java Swing? for details.
Stick to Java naming conventions. Method named Panel() is confusing. See Naming Conventions.
There is a missing else statement in Panel() method. On the second click, you first get into the first if statement, set the color BLUE, then, enter the second if statement and reset the color to GREEN.
As mentioned above in comments, & and && are different operators. See Operators.

+1, there are sooo... many problems for such a simple example. You caught a lot of them. Sure hope the OP reads the `Custom Painting` tutorial. — camickr, Nov 13 '13 at 16:36
Thanks for catching all those errors for me, I'm new to java so this is very helpful to me. — dakatk, Nov 13 '13 at 16:38

score 2 · Accepted Answer · answered Nov 13 '13 at 16:35

What is the point of the Timer? A Timer is used to schedule an event. You are not scheduling any events, you are waiting for the user to click the mouse. There is no need for the Timer. If you ever do need a Timer then:

you would never schedule the Timer to fire every millisecond, that is too fast for the machine to respond to. Use a more realistic value depending on the requirement. Maybe 50 or 100ms.
you would never start the Timer in a painting method. You would probably start the Timer in the constructor of your class.

Your MouseListener code is wrong. You start the Timer and invoke the Panel() method which invokes the addMouseListener() method. So after 1 second you will have added 1000 MouseListeners added to your panel. This is obviously wrong. The MouseListener should be added in the constructor of your class.

There is no need for the "co" variable (if it was needed it would NOT be static). All Swing components support a setForeground() method to set the color of the painted text. In your case you are painting a shape, but you can still use the setForeground() method when you want to change the color. Then in your painting code you would just use:

//g.setColor(co);
g.setColor(getForeground());

Since you added the MouseListener to the panel there is no need to check the point of the mouse click. The event will only be generated when you click on the panel. So all you need to do is use setForeground(...) to the color you want. You don't even need to invoke repaint since Swing is smart enough to repaint itself when you change one of its properties.

Changed it all based on your suggestions, and it works now. Thanks! — dakatk, Nov 13 '13 at 16:56

JPanel not updating on second mouse click

2 Answers2