Do not clear messages list when user clicks No.

[mathieu-bourdin]

The title is pretty self explanatory...

[sharpedavid]

(Not the maintainer. I'm just interested in this.)

I can reproduce this bug, but I couldn't figure out why it occurs because the existing code looks correct to me:

synchronized (SMTPServerHandler.INSTANCE.getMailSaver().getLock()) {
    // Note: Should delete emails before calling observers, since observers will clean the model.
	if (answer == JOptionPane.YES_OPTION) {
		SMTPServerHandler.INSTANCE.getMailSaver().deleteEmails();
	}
    setChanged();
    notifyObservers();
	button.setEnabled(false);
}

But the existing comment actually reveals the bug: "Should delete emails before calling observers, since observers will clean the model." MailsListPane observes ClearAllButton, and it clears the email list regardless of which button was clicked.

(deleteEmails() is called only when the user clicks "Yes". If you click "No", the email list in the GUI is cleared, but the "received-emails" directory still contains the emails.)

The change looks good to me, but I haven't tested it. Thanks for contributing, @mathieu-bourdin.

[mathieu-bourdin]

Thanks for your interest, a more detailed explanation below.

Behaviour on master branch:

• Yes : clears messages list and deletes corresponding eml files.
• No : clears messages list and keeps eml files.

Behaviour on fix-clearall-button-confirmation branch:

• Yes : clears messages list and deletes corresponding eml files. (unchanged)
• No : does nothing.

Now, let's see what happens in ClearAllButton's ActionListener.

On master

if (answer == JOptionPane.CLOSED_OPTION) {
    return;
}

synchronized (SMTPServerHandler.INSTANCE.getMailSaver().getLock()) {
    // Note: Should delete emails before calling observers, since observers will clean the model.
    if (answer == JOptionPane.YES_OPTION) {
        SMTPServerHandler.INSTANCE.getMailSaver().deleteEmails();
    }
    setChanged();
    notifyObservers();
    button.setEnabled(false);
}

Clicking 'No' yields NO which is different from CLOSED_OPTION, so the action handler does not return immediately.
Then in synchronized block, eml files are not delete because answer is not YES_OPTION but notifyObservers() is still called (then MailsListPane clears the table model).

The fix

if (answer != JOptionPane.YES_OPTION) {
    return;
}

synchronized (SMTPServerHandler.INSTANCE.getMailSaver().getLock()) {
    // Note: Should delete emails before calling observers, since observers will clean the model.
    SMTPServerHandler.INSTANCE.getMailSaver().deleteEmails();
    setChanged();
    notifyObservers();
    button.setEnabled(false);
}

Return immediately if answer is not YES_OPTION.
Then in synchronized block: no need to check that answer is YES_OPTION anymore since we returned if it was not :)

By the way, fakeSMTP is a really great tool, and this is just nitpicking. I mean as long as eml files are not deleted when user clicks No... this is more or less a cosmetic issue.

[mathieu-bourdin]

Closed by a missclick :)

[Simulant87]

found the same bug. #70

[Simulant87]

@sharpedavid When you click No, the email are not delted, but the button still notifys the observers about a change (that actually did not happen) leading the listView to be cleared.

The fix is good. But the cause of the bug is acutal a conceptual problem that the Button that triggers an Action is an Observable. Normally the Model and not an Action is Observable.
So in an optimal implementation the ClearAllButton would send an Action to the Model (MailSaver) that would delete the E-Mails, and the ListView would not observe the Button but only the model. The Model would notify the list view, that the state has changed and the ListView would be responsible to read the new state from the Model and update accordingly.
In this implementation of the update loop a notify about a NoOperation change would not have any effect (as expected).