Swing Multithreading. My GUI is freezing

Question

Disclaimer: I'm not using my program for anything malicious, even though it's name is Spambot. I'm only using it for practice.

Edit: My problem is that the GUI freezes if I press a button, and thus I'm unable to press another button until the first one finishes its job. How would I go around this?

I made a class (SpambotGUI), which is basically a JFrame with 3 JButtons in it. Here is the code for it:

public class SpambotGUI extends JPanel implements ActionListener {
    private static final long serialVersionUID = 1L;
    static JButton button1 = new JButton("Spam first file");
    static JButton button2 = new JButton("Spam second file");
    static JButton button3 = new JButton("Stop");

    public SpambotGUI() throws AWTException {
        button1.addActionListener(this);
        button2.addActionListener(this);
        button3.addActionListener(this);
        button1.setActionCommand("spam1");
        button2.setActionCommand("spam2");
        button3.setActionCommand("stop");
        button1.setMnemonic(KeyEvent.VK_F7);
        button2.setMnemonic(KeyEvent.VK_F8);
        button3.setMnemonic(KeyEvent.VK_F9);
        button3.setToolTipText("Stop the program");
        add(button1, BorderLayout.WEST);
        add(button2, BorderLayout.CENTER);
        add(button3, BorderLayout.SOUTH);

    }

    public void actionPerformed(ActionEvent e) {
        System.out.println(java.awt.EventQueue.isDispatchThread());
        if ((e.getActionCommand()).equals("spam1")) {
            try {
                Spambot.Start("data/spambotLines1.txt");
            } catch (FileNotFoundException | AWTException | InterruptedException e1) {
                // TODO Auto-generated catch block
                e1.printStackTrace();
            }
        } else if ((e.getActionCommand()).equals("spam2")) {
                   try {
                       Spambot.Start("data/spambotLines2.txt");
                   } catch (FileNotFoundException | AWTException | InterruptedException e1) {
                    // TODO Auto-generated catch block
                    e1.printStackTrace();
                   }
               } else if ((e.getActionCommand()).equals("stop")) {
                          Spambot.stopped = true;
                          Spambot.thread.interrupt();
                      }

    }

    public static void CreateGUI() throws AWTException {
        JFrame frame = new JFrame();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        SpambotGUI buttons = new SpambotGUI();
        buttons.setOpaque(true);
        frame.setContentPane(buttons);

        frame.pack();
        frame.setVisible(true);
    }

    public static void main(String args[]) throws Exception {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    CreateGUI();
                } catch (AWTException e) {
                    e.printStackTrace();
                }

            }
        });
    }

}

I also have a Spambot class, which consists of the following: In the Start method, I have a loop, containing irrelevant stuff and thread.Sleep-s (I made a new Thread() called thread, that's why it's spelled with a lower case t in SpambotGUI), and the loop is supposed to run until the stopped boolean in Spambot is false. The latter would be set to false if I pressed the Stop button in the GUI. The problem is that while the loop in Start is running, I'm unable to click any of buttons in the GUI. I read after this on the internet, and I came to the conclusion that I should be using multi-threading here.

The thing is, I just can't figure out how it's supposed to work. I tried implementing Runnable in my Spambot class and then calling the run() method from SpambotGUI, but nothing changed.

Does anyone know what I should be doing here?

Edit: Here's a part of the Spambot class:

public class Spambot{

    private static Robot robot;
    public static Thread thread = new Thread();
    public static boolean stopped = false;

    public static void main(String... args) throws Exception {

    }

    public static void Start(String path) throws AWTException, InterruptedException, FileNotFoundException {
        Scanner input = new Scanner(new FileReader(path));
        Spambot keyboard = new Spambot();
        Random rand = new Random();
        robot.keyPress(KeyEvent.VK_ALT);
        thread.sleep(150);
        robot.keyPress(KeyEvent.VK_TAB);
        thread.sleep(150);
        robot.keyRelease(KeyEvent.VK_TAB);
        robot.keyRelease(KeyEvent.VK_ALT);
        thread.sleep(500);
        while (input.hasNextLine() && !stopped) {
            keyboard.type(input.nextLine());
            thread.sleep(rand.nextInt(1500)+1000);
            robot.keyPress(KeyEvent.VK_ENTER);
            robot.keyRelease(KeyEvent.VK_ENTER);
        }
        input.close();
    }


    public Spambot() throws AWTException {
        Spambot.robot = new Robot();
    }

    public Spambot(Robot robot) {
        Spambot.robot = robot;
    }
}

Answer 1

You might actually have to launch a new Thread, so the blocking operations do not affect your application GUI too much. However, operations within that update the GUI should be executed by the original Event Dispatch Thread.

As pointed in other answers, here the main offender seems to be the use of Thread.sleep(). Which when executed in the Event Dispatch Thread will cause the GUI to become irresponsive (won't accept your input or redraw until it has finished executing your event listener code). However that Thread.sleep() is acceptable if used in other Threads (which wont freeze your GUI).

How to do it

First: Launch your blocking handling code in a separate Thread.

public void actionPerformed(ActionEvent e) {
    if ((e.getActionCommand()).equals("spam1")) {
        new Thread(){
            @Override
            public void run() {
                try {
                    Spambot.Start("data/firstfile.txt");
                } catch (FileNotFoundException | InvocationTargetException | 
                        AWTException | InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }.start();
    }
    // ... rest of conditions

}

Second, every individual GUI update between delays, should be done in the Event Dispatch Thread.

EventQueue.invokeAndWait(new Runnable(){
    public void run() {
        robot.keyPress(KeyEvent.VK_ALT);
    };
});

As all the updates done are in robot.keyPress() invocations, a good choice might be to encapsulate and re-use in a method. Note that local variables and arguments that are used inside inner class should be defined as final (so they are made available outside the stackframe of the method)

private static void invokeRobotInAWT(final Integer ... ke) throws InvocationTargetException, InterruptedException {
    EventQueue.invokeAndWait(new Runnable(){
        public void run() {
            for (int currentEvent : ke) {
                robot.keyPress(currentEvent);
            }
        };
    });
}

public static void Start(String path) throws AWTException, InterruptedException, FileNotFoundException, InvocationTargetException {
    try (Scanner input = new Scanner(new FileReader(path));) {
        Spambot keyboard = new Spambot();
        Random rand = new Random();
        invokeRobotInAWT(KeyEvent.VK_ALT);
        Thread.sleep(150);
        invokeRobotInAWT(KeyEvent.VK_TAB);
        Thread.sleep(150);
        invokeRobotInAWT(KeyEvent.VK_TAB, KeyEvent.VK_ALT);
        Thread.sleep(500);
        while (input.hasNextLine() && !stopped) {
            // @@@ Would need extra threading here?
            keyboard.type(input.nextLine());
            Thread.sleep(rand.nextInt(1500)+1000);
            invokeRobotInAWT(KeyEvent.VK_ENTER, KeyEvent.VK_ENTER);
        }
    } finally {
        // input closed by try-with-resources
    }
}

Edited: Oops. I got it wrong with SwingWorker. Might actually be adequate.

Note: There are helper components in Swing that might spare us from complex and error-prone thread handling. You might actually use a SwingWorker where your overridden doInBackground() method (in the worker thread) traverses the file, does the pauses, and sends the keystrokes (call publish(Integer)), to be processed by the EDT in the overridden process(List<Integer>) method.

Answer 2

You are not using any threading instead you just using a Thread.sleep() method. Its just a normal program with sleep. So your GUI will be blocked until your operation completes in start1 and start2.

Answer 3

I think it is important that you understand that your program logic and GUI should never be run on the same thread at all. When your program is busy preforming tasks you do not want your GUI to freeze up. The way Java solves this is the Event Dispatch Thread (EDT). You do this like so:

public static void main(String args[]) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            //Your GUI code here
        }
    });
}

More info on the EDT here: https://docs.oracle.com/javase/tutorial/uiswing/concurrency/dispatch.html

Answer 4

What you should be using in any GUI application is not multithreading, but event-driven programming. That means never execute long-running loops inside an event handler, and never, ever, call Thread.sleep in one.

Instead, a delayed GUI action must be scheduled on a Swing Timer, and the loop body must be put into the handler which you submit to the timer.

If you don't have a delayed action, but truly a long-running task (that would mean you do a lot of computation or wait for I/O), only then would you need a background thread to do the work. In that case you would use the SwingWorker to communicate the task's results back to the GUI.