Denny DVD - DatabaseLocationDialog

[Sean Keane]

I am looking at the code that Andrew Monkhouse wrote for his book and I am confused about something in the DatabaseLocationDialog class. Maybe someone familiar with this code can help?

My question is - why does DatabaseLocationDialog need to respond to the windowClosing event?

Andrew makes his class DatabaseLocationDialog extend WindowAdapter in order to respond to the windowClosing event.

The DatabaseLocationDialog class is used to get the location of the database file. The class itself is not a dialog. The class launches a dialog in the constructor:

options = new JOptionPane(configOptions, JOptionPane.QUESTION_MESSAGE, JOptionPane.OK_CANCEL_OPTION);
dialog = options.createDialog(parent, TITLE);
dialog.setDefaultCloseOperation(JDialog.DO_NOTHING_ON_CLOSE);
dialog.addWindowListener(this);

So you can see that the dialog that is created is set to do nothing upon close. I'm not sure why he did this? Why not just set it to be EXIT_ON_CLOSE?

I am guessing since he set the dialog to be DO_NOTHING_ON_CLOSE that this is why he is responding to the windowClosing event. The related methods are:

public void windowClosing(WindowEvent we) {
        processCommand(EXIT);
    }
private void processCommand(String command) {
        dialog.setVisible(false);
        if (CONNECT.equals(command)) {
            options.setValue(JOptionPane.OK_OPTION);
        } else {
            options.setValue(JOptionPane.CANCEL_OPTION);
        }
    }

So you can that when the EXIT command is passed into the processCommand method, it results in this line being executed:

options.setValue(JOptionPane.CANCEL_OPTION);

So closing the dialog is the same as clicking on the cancel button it would seem? This would seem to answer my first question why does DatabaseLocationDialog need to respond to the windowClosing event? - he is doing this as he wants to handle closing the window in the same way he handles the user clicking on the cancel button. Does this sound correct?

So where is the code that actually says that tells the JOptionPane that it is no longer needed? It seems that it is just made invisible and never actually destroyed?

Or does the call to

options.setValue(JOptionPane.CANCEL_OPTION);

actually cause the JOptionPane to be destroyed?

[Roel De Nijs]

Sean Keane wrote:Does this sound correct?

Sounds correct to me (and also understandable to take such an approach).

Sean Keane wrote:So where is the code that actually says that tells the JOptionPane that it is no longer needed? It seems that it is just made invisible and never actually destroyed?

No idea Maybe just GC'ed because the variable is not referenced anymore.

[Sean Keane]

It's taken me quite a while to get my head around Andrew's GUI solution. It seems overly complicated for what it is doing. I reckon I will go with something a little simpler.

For example, there doesn't seem to be any benefit with catching the windowClosing event as he doesn't do anything when the window is closing. So setting the dialog to EXIT_ON_CLOSE would seem to do the same thing but with a lot less code and less complexity. Unless of course I am missing something, hmmm...

[S. Thakker]

Hi Seane,

1. I think what he is trying to do is just as you said, execute a standard exit operation when teh X, Quit etc is
clicked. It looks like he is also trying to re-use the dialog box... But I dont think he needed to even provide that type
of functionailty for this dialog. It overkill. I thnk that if we were talking about the main window, would make much more sense.

For example: When I handle closing events, I usually declare something like this:

private class Quit extends WindowAdapter implements ActionListener { }

I then implement a standard function which all the events are routed to. This makes every point
of exit perform the same cleanup, shutdown etc.. that needs to be done.

The thing is, he does a setVisible(false)... I think his intention was to reuse this dialog and Setting EXIT_ON_CLOSE or HIDE_ON_CLOSE
would have accomplished the same thing... maybe in the future t his dialog will do much more.... not really sure.

[Sean Keane]

Ah, I should have read the book . On page 284 Andrew has the following:

If the user closes the dialog box rather than clicking the Connect or Exit button, we want to treat it as though they had clicked the Exit button. So we set the dialog to do nothing on close, and then we add a window listener to the dialog box

So that is an explanation that matches my understanding of what he was doing. But it would have been useful to include a reason as to why he chose this approach. Because it doesn't seem to make a lot of sense that all he is doing when the window is closed is to set it to be invisible.

I've a feeling he may have had a good reason for doing this, rather than letting the JOptionPane exit in the normal fashion, so wondering if anyone has any ideas?

By way of contrast. In his code for the Server window he uses a System.exit(0) for the Quit menu item and he sets the Window to use EXIT_ON_CLOSE. I would have expected him to do the same for this dialog i.e. the Cancel button should have mapped to a System.exit(0) and the dialog itself to have used EXIT_ON_CLOSE.

Any ideas?

[Roel De Nijs]

I don't think it's really important, but I'll give it a go.

Maybe he just wants to show different alternatives to implement the closing of a dialog (it's an instructive book). Maybe he just wants to confuse the user a bit and make him/her think instead of doing some copying and pasting (I would say mission accomplished ) Maybe there is another reason.

[Sean Keane]

I am just debugging through the code at the moment. I am running the application in stand alone mode and I noticed an interesting thing. It appears that the code is not thread safe - there is a race condition somewhere.

I was looking at this bit of code here from the constructor of the MainWindow class:

        DatabaseLocationDialog dbLocation =  new DatabaseLocationDialog(this, connectionType);

        if (dbLocation.userCanceled()) {
        	System.exit(0);
        }
        try {
            controller = new GuiController(dbLocation.getNetworkType(), dbLocation.getLocation(), dbLocation.getPort());
        } catch (GuiControllerException gce) {
            ApplicationRunner.handleException("Failed to connect to the database");
        }

Now, if you close the dialog that is looking for the database file location you should never get to the try\catch block where the GuiController is created. However, as I was debugging through the code, on some occasions when I closed the dialog window the code went on to attempt to create the GuiController.

The only explanation I can think of for this is that the code is not thread safe and there is a race condition. There are two threads at play here. The first is the thread that is running the MainWindow class. The second is the Event Dispatch Thread that is handling the event which is fired when the dialog is closed.

So the call dbLocation.userCanceled() is made on one thread and the setting of this value is done on another thread. I wonder should some locking be used here in order to guarantee that no race condition occurs?

[Roel De Nijs]

Sean Keane wrote:I wonder should some locking be used here in order to guarantee that no race condition occurs?

Let's say you would enter a wrong database file location and then you hit cancel. You don't want to see some exception telling you database file is not a valid one if you press cancel.
But I didn't debug that part of my application, so the problem may also occur in mine, I don't have any idea about it. Or maybe by having another approach I didn't had to handle this issue at all.

[Sean Keane]

I think I just discovered where the problem is. The problem is when running in standalone mode, if you click on the Exit button on the dialog for specifying the location of the database file, then the code may run ahead and attempt to create the GuiController. There is a race condition - some times it will attempt to create the GuiControllerand thus throw an exception causing the application to hang, other times it will not attempt to create the GuiController and the application will exit normally.

It looks like the problem bit of code is here in the DatabaseLocationDialog class:

private void processCommand(String command) {
        dialog.setVisible(false);
        if (CONNECT.equals(command)) {
            options.setValue(JOptionPane.OK_OPTION);
        } else {
            options.setValue(JOptionPane.CANCEL_OPTION);
        }
    }

This method is called both when the Exit button is clicked and when the Window is closed. However things operate differently on both occasions.

There are a few threads running here. Two of interest are the main thread is running the MainWindow class and secondly the thread that is handling the event that has been trigged in the dialog window. The dialog window itself is running in a separate thread to the thread that the MainWindow constructor is running in.

The main thread is blocked and left in a waiting state by the JOptionPane i.e. the dialog window. So in the constructor of the MainWindow we have:

DatabaseLocationDialog dbLocation =
                new DatabaseLocationDialog(this, connectionType);
        // The code is blocked here until the dialog that is created in the constructor of 
        // DatabaseLocationDialog allows us to continue.
        if (dbLocation.userCanceled()) {
        	System.exit(0);
        }

Remember that in the last few lines of constructor of the DatabaseLocationDialog class we have:

        dialog.setDefaultCloseOperation(JDialog.DO_NOTHING_ON_CLOSE);
        dialog.addWindowListener(this);
        dialog.setVisible(true);
    }

The call to dialog.setVisible(true); puts the thread that is running the DatabaseLocationDialog class into a waiting state. Of course this is the same thread that is running the MainWindow class (as the MainWindow class invoked the constructor of the DatabaseLocationDialog class).

When you close the window, using the X button, everything works correctly.

The EventDispatchThread calls the processCommand method in the DatabaseLocationDialog class.

The processCommand method sets the JOptionPane to indicate that the user has chosen the CANCEL_OPTION (remember Andrew is treating click on the Cancel button and closing the window as the same).

The JOptionPane now lets go of some lock it has, and it notifies all waiting threads.

The constructor of the DatabaseLocationDialog class receives notification that the JOptionPane has finished. So the constructor for DatabaseLocationDialog now completes and return is handed back to the MainWindow class.

The MainWindow class now goes on to call dbLocation.userCanceled().

The userCanceled method returns true to indicate that the user cancelled the dialog.

The program exits with a System.exit(0).

The key point in the list of above steps is that the JOptionPane holds on to a lock and does not release this lock until after the processCommand method has been called.

However, when you close the dialog window using the Exit button, things work differently. The lock that the JOptionPane holds, which is preventing the MainWindow constructor continuing, is released before the call to processCommand.

If you put a break point on the first line of the processCommand method you will see that the thread that the MainWindow constructor class is running in becomes alive. You can actually go to this thread and step over the code, and you will see that the call to dbLocation.userCanceled() returns false!!!

So it appears that there is a flaw here. I wonder how it should be fixed. You could put the call to dbLocation.userCanceled() in the constructor of the MainWindow class on the Event Dispatch Thread maybe by using SwingUtilities.invokeLater(...)? Not sure if that is a nice solution.

[Roel De Nijs]

That's some nice debugging you did I'm not a real Swing expert, so I don't have an idea about what could be a nice solution to solve this issue. But I doubt if using invokeLater is the way to go. I don't know if you have the guarantee if it's executed after processCommand is completed.

[Sean Keane]

Ah, I found a solution to the problem. In the ApplicationRunner class Andrew simply launches the MainWindow by doing the following:

if (args.length == 0 || "alone".equalsIgnoreCase(args[0])) {
            // Create an instance of the main application window
            new MainWindow(args);
        } else if ("server".equalsIgnoreCase(args[0])) {
            new ServerWindow();
        } else {}

Now if you instead run the MainWindow on the EventDispatchThread as follows then you can remove this race condition:

        if (args.length == 0 || "alone".equalsIgnoreCase(args[0])) {
            // Create an instance of the main application window
            SwingUtilities.invokeLater(new Runnable() {                
                public void run() {
                    new MainWindow(args);     
                }
            });            
        }

I've tested this in Eclipse by placing a break point at the first line of this method from the DatabaseLocationDialog class:

    private void processCommand(String command) {
        dialog.setVisible(false);
        if (CONNECT.equals(command)) {
            options.setValue(JOptionPane.OK_OPTION);
        } else {
            options.setValue(JOptionPane.CANCEL_OPTION);
        }
    }

Previously, when you executed the first line of this method (after clicking on the Exit button), the thread that was running the MainWindow class would become alive again and we had a race condition.

Now, after my change, when you execute the first line of this method, the MainWindow class does not become alive. So we are guaranteed that we able to set the value of the JOptionPane before the MainWindow class attempts to read the value of the JOptionPane (through the call to dbLocation.userCanceled()). Problem solved .

This is interesting. Because originally in my solution I had the invocation of the MainWindow and Server window wrapped in SwingUtilities.invokeLater(...). Then I convinced myself that they were not needed. I thought there were not needed as I read somewhere that you only needed to use SwingUtilities.invokeLater(...) when you were carrying out operations on GUI components after the GUI was displayed. Obviously this is not true and it was probably a very simplistic rule to try and help people decide when to use this method. But now I think I will switch back to wrapping them in SwingUtilities.invokeLater(...).

It just goes to show how tricky this multi-threaded code can be - causing you problems in places where you may not think it would

I wonder is Andrew or some GUI expert around to comment. I would love to know if my fix is the correct solution and why we had the problem in the first place?

[S. Thakker]

Hi Seane,

I think by setting the JOptionPane.setValue(OK), releases control and lets the main thread continue. I think another option
to use would be to set a flag before.... I know you have your example all set up, so if you dont mind giving this a try..

I am assuming since this code doesnt set a flag, he is calling the JOptionPane.getValue() to get the state....
and without looking at the code for the setValue() method, I am not sure at what point the control is released and at
what point the value is set and available for retrieval from the getValue method.

   private void processCommand(String command) {   
        dialog.setVisible(false);   
        if (CONNECT.equals(command)) {   
            options.setValue(JOptionPane.OK_OPTION);   
        } else {   
            options.setValue(JOptionPane.CANCEL_OPTION);   
        }   
    }  

try this.....

boolean cancelFlag = false;

private void processCommand(String command) {   
        dialog.setVisible(false);   
        if (CONNECT.equals(command)) {   
            cancelFlag = false;
            options.setValue(JOptionPane.OK_OPTION);   
        } else {   
            cancelFlag = true;
            options.setValue(JOptionPane.CANCEL_OPTION);   
        }   
    }  


public boolean userCanceled() {
  return cancelFlag;
}

[Darryl Burke]

Sean. you seem to be hung up on

Why not just set it to be EXIT_ON_CLOSE?

Read the API for JDialog#setDefaultCloseOperation(...). What are the valid parameters?

[Roel De Nijs]

Sean Keane wrote:Why not just set it to be EXIT_ON_CLOSE?

Because that will result in Java 6 in an IllegalArgumentException (see this bug report)

[Sean Keane]

Darryl Burke wrote:Sean. you seem to be hung up on

Why not just set it to be EXIT_ON_CLOSE?

Read the API for JDialog#setDefaultCloseOperation(...). What are the valid parameters?

I'm not hung up on it. Just trying to understand it . Maybe it was not clear, so allow me to explain...

The main thing I was trying to understand was why the code is set up to do nothing when dialog is closed rather than disposing of it.

Looking at the JavaDoc there is no EXIT_ON_CLOSE. So what I should have said was why not just use DISPOSE_ON_CLOSE.

Now we are back to the original thing I was trying to understand. Why not just dispose of the dialog? Why is the code set up to do nothing when the dialog is closed, when the code that is handling the windowClosed event is simply hiding the dialog. Hiding the dialog is the default behaviour on closing.

So on the face of it, it seems like additional code and complexity was added for precisely the same behaviour as the default behaviour of JDialog when it is closed. Which makes me question why this additional code\complexity was added in the first place.

However, because I am new to GUI coding, I am open to the idea that I don't fully understand what the author of the code was doing so there may be something in here I can learn.

I can appreciate to someone who is more knowledgeable about GUI coding that this may seem like an obvious or simple question. But we all think that about an area we are knowledgeable about - everything makes sense once you know it ;-)