Why is my download progress bar firing the same event multiple times?

Question

I'm practicing Swing and I coded a download progress bar to download an image when the user presses the "Start download" button. The download works. The problem is that in my Terminal, I can see that the same event (propertyChange) is being fired multiple times, the number of times increasing with every subsequent download. I've debugged my code with checkpoints, but I'm still not sure why this is happening.

To be more specific, in my Terminal I'm seeing something like

...100% completed 
...100% completed 
...100% completed 
...100% completed 
...100% completed 
...100% completed 
...100% completed

when I expect to see "...100% completed" only once. The number of "...100% completed" that is displayed accumulates with every download. I'm not sure if this is affecting the performance of my download, but I'm wondering why it's happening.

ProgressBar.java:

package download_progress_bar;

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

public class ProgressBar {
    private JFrame frame;
    private JPanel gui;
    private JButton button;
    private JProgressBar progressBar;

    public ProgressBar() {
        customizeFrame();
        createMainPanel();
        createProgressBar();
        createButton();
        addComponentsToFrame();
        frame.setVisible(true);
    }

    private void customizeFrame() {
        // Set the look and feel to the cross-platform look and feel
        try {
            UIManager.setLookAndFeel(UIManager.getCrossPlatformLookAndFeelClassName());
        } catch (Exception e) {
            System.err.println("Unsupported look and feel.");
            e.printStackTrace();
        }

        frame = new JFrame();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setResizable(false);
    }

    private void createMainPanel() {
        gui = new JPanel();
        gui.setLayout(new BorderLayout());
    }

    private void createProgressBar() {
        progressBar = new JProgressBar(0, 100);
        progressBar.setStringPainted(true);  // renders a progress string
    }

    private void createButton()  {
        button = new JButton("Start download");
    }

    private void addComponentsToFrame() {
        gui.add(progressBar, BorderLayout.CENTER);
        gui.add(button, BorderLayout.SOUTH);
        frame.add(gui);
        frame.pack();
    }

    // Add passed ActionListener to the button
    void addButtonListener(ActionListener listener) {
        button.addActionListener(listener);
    }

    // Get progress bar
    public JProgressBar getProgressBar() {
        return progressBar;
    }

    // Enable or disable button
    public void turnOnButton(boolean flip) {
        button.setEnabled(flip);
    }
}

Downloader.java:

package download_progress_bar;

import java.net.*;
import java.io.*;
import java.beans.*;

public class Downloader {
    private URL url;
    private int percentCompleted;
    private PropertyChangeSupport pcs;

    public Downloader() {
        pcs = new PropertyChangeSupport(this);
    }

    // Set URL object
    public void setURL(String src) throws MalformedURLException {
        url = new URL(src);
    }

    // Add passed PropertyChangeListener to pcs
    public void addListener(PropertyChangeListener listener) {
        pcs.addPropertyChangeListener(listener);
    }

    public void download() throws IOException {
        // Open connection on URL object
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();

        // Check response code (always do this first)
        int responseCode = connection.getResponseCode();
        System.out.println("response code: " + responseCode);
        if (responseCode == HttpURLConnection.HTTP_OK) {
            // Open input stream from connection
            BufferedInputStream in = new BufferedInputStream(connection.getInputStream());
            // Open output stream for file writing
            BufferedOutputStream out = new BufferedOutputStream(new FileOutputStream("cat.jpg"));

            int totalBytesRead = 0;
            //int percentCompleted = 0;
            int i = -1;
            while ((i = in.read()) != -1) {
                out.write(i);
                totalBytesRead++;

                int old = percentCompleted;
                percentCompleted = (int)(((double)totalBytesRead / (double)connection.getContentLength()) * 100.0);
                pcs.firePropertyChange("downloading", old, percentCompleted);

                System.out.println(percentCompleted);  // makes download a bit slower, comment out for speed
            }

            // Close streams
            out.close();
            in.close();
        }
    }
}

Controller.java:

package download_progress_bar;

import java.util.concurrent.ExecutionException;
import javax.swing.*;
import java.awt.event.*;
import java.util.List;
import java.net.*;
import java.io.*;
import java.beans.*;

public class Controller {
    private ProgressBar view;
    private Downloader model;
    private JProgressBar progressBar;
    private SwingWorker<Void, Integer> worker;

    public Controller(ProgressBar theView, Downloader theModel) {
        view = theView;
        model = theModel;
        progressBar = view.getProgressBar();

        // Add button listener to the "Start Download" button
        view.addButtonListener(new ButtonListener());
    }

    class ButtonListener implements ActionListener {
        /**
         * Invoked when user clicks the button.
         */
        public void actionPerformed(ActionEvent evt) {
            view.turnOnButton(false);
            progressBar.setIndeterminate(true);
            // NOTE: Instances of javax.swing.SwingWorker are not reusable, 
            // so we create new instances as needed
            worker = new Worker();
            worker.addPropertyChangeListener(new PropertyChangeListener() {
                @Override
                public void propertyChange(PropertyChangeEvent evt) {
                    if (evt.getPropertyName().equals("progress")) {
                        progressBar.setIndeterminate(false);
                        progressBar.setValue(worker.getProgress());
                    }
                }
            });
            worker.execute();
        }
    }

    class Worker extends SwingWorker<Void, Integer> implements PropertyChangeListener {
        /* 
         * Download task. Executed in worker thread.
         */
        @Override
        protected Void doInBackground() throws MalformedURLException {
            model.addListener(this);
            try {
                String src = "https://lh3.googleusercontent.com/l6JAkhvfxbP61_FWN92j4ulDMXJNH3HT1DR6xrE7MtwW-2AxpZl_WLnBzTpWhCuYkbHihgBQ=s640-h400-e365";
                model.setURL(src);
                model.download();
            } catch (IOException ex) {
                System.out.println(ex);
                this.cancel(true);
            }   
            return null;
        }

        /*
         * Executed in event dispatching thread
         */
        @Override
        protected void done() {
            try {
                if (!isCancelled()) {
                    get();  // throws an exception if doInBackground throws one
                    System.out.println("File has been downloaded successfully!");
                }
            } catch (InterruptedException x) {
                x.printStackTrace();
                System.out.println("There was an error in downloading the file.");
            } catch (ExecutionException x) {
                x.printStackTrace();
                System.out.println("There was an error in downloading the file.");
            }

            view.turnOnButton(true);
        }

        /**
         * Invoked in the background thread of Downloader.
         */
        @Override
        public void propertyChange(PropertyChangeEvent evt) {
            this.setProgress((int) evt.getNewValue());
            System.out.println("..." + this.getProgress() + "% completed");
        }
    }
}

Main.java:

package download_progress_bar;

import javax.swing.SwingUtilities;

/**
 * Runs the download progress bar application.
 */
public class Main {
    public static void main(String[] args) {
        // Schedule a job for the event-dispatching thread:
        // creating and showing this application's GUI.
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                // Create view
                ProgressBar view = new ProgressBar();
                // NOTE: Should model/controller be created outside invokeLater?
                // Create model
                Downloader model = new Downloader();
                // Create controller
                Controller controller = new Controller(view, model);
            }
        });
    }
}

EDIT: I've updated my code to reflect the suggested changes. But even after making the changes, the problem persists. I am still seeing multiple invocations of "...100% completed", the number of invocations increasing with every subsequent download. For example, I run the application and press the download button for the first time, I will see

...100% completed

I press the download button again. I see

...100% completed
...100% completed

I press the download button again...

...100% completed
...100% completed
...100% completed

and so on. Why is this happening?

Answer 1

It's possible that, because of the way the percentage is calculated that it will report 100% when there is still some more work to be completed

During my testing I observed...

//...
98
...
99
99
...
100

So a lot of the values were repeated before the code completed.

I noted some issues/oddities in your download code, mostly the fact that you completely ignore the percentCompleted property, so I changed it to something more like...

public void download() throws IOException {
    // Open connection on URL object
    HttpURLConnection connection = (HttpURLConnection) url.openConnection();

    // Check response code (always do this first)
    int responseCode = connection.getResponseCode();
    System.out.println("response code: " + responseCode);
    if (responseCode == HttpURLConnection.HTTP_OK) {
        // Open input stream from connection
        BufferedInputStream in = new BufferedInputStream(connection.getInputStream());
        // Open output stream for file writing
        BufferedOutputStream out = new BufferedOutputStream(new FileOutputStream("cat.jpg"));

        int totalBytesRead = 0;
        //int percentCompleted = 0;
        int i = -1;
        while ((i = in.read()) != -1) {
            out.write(i);
            totalBytesRead++;

            int old = percentCompleted;
            percentCompleted = (int) (((double) totalBytesRead / (double) connection.getContentLength()) * 100.0);
            pcs.firePropertyChange("downloading", old, percentCompleted);

            System.out.println(percentCompleted);  // makes download a bit slower, comment out for speed
        }

        // Close streams
        out.close();
        in.close();
    }
}

For me, I'd change the code slightly, instead of doing...

@Override
protected void process(List<Integer> chunks) {
    int percentCompleted = chunks.get(chunks.size() - 1); // only interested in the last value reported each time
    progressBar.setValue(percentCompleted);

    if (percentCompleted > 0) {
        progressBar.setIndeterminate(false);
        progressBar.setString(null);
    }
    System.out.println("..." + percentCompleted + "% completed");
}

/**
 * Invoked when a progress property of "downloading" is received.
 */
@Override
public void propertyChange(PropertyChangeEvent evt) {
    if (evt.getPropertyName().equals("downloading")) {
        publish((Integer) evt.getNewValue());
    }
}

You should take advantage of SwingWorkers inbuilt progress support, for example...

/**
 * Invoked when a progress property of "downloading" is received.
 */
@Override
public void propertyChange(PropertyChangeEvent evt) {
    setProgress((int)evt.getNewValue());
}

This will mean you will need to attached a PropertyChangeListener to the SwingWorker

/**
 * Invoked when user clicks the button.
 */
public void actionPerformed(ActionEvent evt) {
    view.turnOnButton(false);
    progressBar.setIndeterminate(true);
    // NOTE: Instances of javax.swing.SwingWorker are not reusable, 
    // so we create new instances as needed
    worker = new Worker();
    worker.addPropertyChangeListener(new PropertyChangeListener() {
        @Override
        public void propertyChange(PropertyChangeEvent evt) {
            if ("progress".equals(evt.getPropertyName())) {
                progressBar.setIndeterminate(false);
                progressBar.setValue(worker.getProgress());
            }
        }
    });
    worker.execute();
}

The side effect to this is, you know have a means to also be notified when the SwingWorker's state changes to check to see when it is DONE

Updated

Okay, after going over the code, again, I can see that you're adding a new PropertyChangeListener to model EVERY TIME you execute the SwingWorker

/* 
 * Download task. Executed in worker thread.
 */
@Override
protected Void doInBackground() throws MalformedURLException, InterruptedException {
    model.addListener(this); // Add another listener...
    try {
        String src = "https://lh3.googleusercontent.com/l6JAkhvfxbP61_FWN92j4ulDMXJNH3HT1DR6xrE7MtwW-2AxpZl_WLnBzTpWhCuYkbHihgBQ=s640-h400-e365";
        model.setURL(src);
        model.download();
    } catch (IOException ex) {
        System.out.println(ex);
        this.cancel(true);
    }
    return null;
}

Because the model is an instance field of Controller, this is having an accumulative effect.

One solution might be to just add the Downloader as the listener to the model, but that would require you to ensure that any updates you perform to the UI are synced properly.

A better, general, solution would be to add support to remove the listener once the worker completes

public class Downloader {
    //...        
    public void removeListener(PropertyChangeListener listener) {
        pcs.removePropertyChangeListener(listener);
    }

And then in the SwingWorkers done method, remove the listener...

/*
 * Executed in event dispatching thread
 */
@Override
protected void done() {
    model.removeListener(this);

Answer 2

As shown here and here, SwingWorker maintains two bound properties: state and progress. Invoking setProgress() ensures that "PropertyChangeListeners are notified asynchronously on the Event Dispatch Thread." Simply add a PropertyChangeListener to your progress bar and call setProgress() in your implementation of doInBackground(), or a method that it calls such as download(). Conveniently, "For performance purposes, all these invocations are coalesced into one invocation with the last invocation argument only."