Closing dialog needs to be equal to clicking the Cancel, not the No button

[pbodnar]

The ConfirmDialog should follow the "standard" yes/no/cancel dialogs behavior: when one presses Esc or clicks on the close icon in the dialog, the dialog's method isCancelled() needs to return true. Currently, it always return false (so the client code thinks "No" was pressed) due to setting the null-able isConfirmed variable without looking at whether we are showing the yes/no/cancel variant of the dialog. The listener which needs to be fixed in DefaultConfirmDialogFactory.java:

    // Close listener implementation
    confirm.addCloseListener(new Window.CloseListener() {

        private static final long serialVersionUID = 1971800928047045825L;

        public void windowClose(CloseEvent ce) {

            // Only process if still enabled
            if (confirm.isEnabled()) {
                confirm.setEnabled(false); // avoid double processing
                confirm.setConfirmed(false);
                if (confirm.getListener() != null) {
                    confirm.getListener().onClose(confirm);
                }
            }
        }
    });

The fix (adding one "if") seems to be very easy to do...

[ingoschaefer]

Well, I think the problem is a bit deeper in the Code. The internal ClickListener on all Buttons is the same on every button and at least in 2.1.3 not handling the not-ok button in any way. So there is currently no way to find out if cancel or not-ok was clicked. I have found as a workaround to remove this quoted CloseListener right after the call of show() and add your own one.

[pbodnar]

Well, I think the problem is a bit deeper in the Code.

Thanks for your update on this topic, although I still think the problem reported is trivial:

The internal ClickListener on all Buttons is the same on every button and at least in 2.1.3 not handling the not-ok button in any way.

I propose you to look at the actual code once again. You can see the ClickListener is the same, but it behaves differently for various buttons being clicked (which is only logical). Most importantly:

                Button b = event.getButton();
                if (b != cancel)
                    confirm.setConfirmed(b == ok);

So there is currently no way to find out if cancel or not-ok was clicked.

Actually it is, you simply call confirm.isCancelled() (returns isConfirmed == null) to find that out in your client code and that works as expected when one of buttons is clicked.

I have found as a workaround to remove this quoted CloseListener right after the call of show() and add your own one.

Well, of course, that's the only way now until the CloseListener is fixed, but the question here from he very beginning is how the listener should look like. I don't have the environment to try out now, but apparently all that should be needed is to change this line:

confirm.setConfirmed(false);

to:

confirm.setConfirmed(threeWay ? null : false);

... or maybe remove this line altogether to be really consistent with how the dialog behaves when cancel button is clicked (i. e. leaving null in the isConfirmed flag).

[ingoschaefer]

You're right, I have not noticed the null value for the cancel-Button before (and the special behaviour of the isConfirmed-Method)

I think the CloseListener should not set the value of confirmed in any way. The CloseListener should only do anything on Keyboard shortcut or the window-close Button and if it is triggered by the close()-Method in the ClickListener it would not do anything harmful because of the isEnabled-Check. And as the default of confirmed is null it would stay this.

[pbodnar]

OK, thanks for your confirmation! :)

So it's up to the author(s) now to remove that problematic line, or to explain to us the reason for wanting it there...