From a656a61c65b368d58479328f24cd4d7a93aaabdf Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sun, 22 Feb 2015 16:58:46 +0000 Subject: [PATCH 1/2] tidy method --- .../setup/AccountSetupCheckSettings.java | 109 ++++++++++-------- 1 file changed, 63 insertions(+), 46 deletions(-) diff --git a/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index 10809631d..99c535abf 100644 --- a/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -24,6 +24,7 @@ import com.fsck.k9.fragment.ConfirmationDialogFragment; import com.fsck.k9.fragment.ConfirmationDialogFragment.ConfirmationDialogFragmentListener; import com.fsck.k9.mail.AuthenticationFailedException; import com.fsck.k9.mail.CertificateValidationException; +import com.fsck.k9.mail.MessagingException; import com.fsck.k9.mail.Store; import com.fsck.k9.mail.Transport; import com.fsck.k9.mail.store.webdav.WebDavStore; @@ -391,10 +392,10 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList } } - class CheckAccountTask extends AsyncTask { + private class CheckAccountTask extends AsyncTask { private final Account account; - CheckAccountTask(Account account) { + private CheckAccountTask(Account account) { this.account = account; } @@ -402,58 +403,36 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList protected Void doInBackground(CheckDirection... params) { final CheckDirection direction = params[0]; try { - if (mDestroyed) { + /* + * This task could be interrupted at any point, but network operations can block, + * so relying on InterruptedException is not enough. Instead, check after + * each potentially long-running operation. + */ + if (cancelled()) { return null; - } - if (mCanceled) { - finish(); - return null; - } - final MessagingController ctrl = MessagingController.getInstance(getApplication()); - ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, - account, direction); + } else { + final MessagingController ctrl = MessagingController.getInstance(getApplication()); + ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, + account, direction); - if (direction == CheckDirection.INCOMING) { - Store store = account.getRemoteStore(); - if (store instanceof WebDavStore) { - publishProgress(R.string.account_setup_check_settings_authenticate); - } else { - publishProgress(R.string.account_setup_check_settings_check_incoming_msg); + if (direction == CheckDirection.INCOMING) { + checkIncoming(); } - store.checkSettings(); + } - if (store instanceof WebDavStore) { - publishProgress(R.string.account_setup_check_settings_fetch); - } - MessagingController.getInstance(getApplication()).listFoldersSynchronous(account, true, null); - MessagingController.getInstance(getApplication()) - .synchronizeMailbox(account, account.getInboxFolderName(), null, null); - } - if (mDestroyed) { + if (cancelled()) { return null; + } else if (direction == CheckDirection.OUTGOING) { + checkOutgoing(); } - if (mCanceled) { + + if (cancelled()) { + return null; + } else { + setResult(RESULT_OK); finish(); - return null; } - if (direction == CheckDirection.OUTGOING) { - if (!(account.getRemoteStore() instanceof WebDavStore)) { - publishProgress(R.string.account_setup_check_settings_check_outgoing_msg); - } - Transport transport = Transport.getInstance(K9.app, account); - transport.close(); - transport.open(); - transport.close(); - } - if (mDestroyed) { - return null; - } - if (mCanceled) { - finish(); - return null; - } - setResult(RESULT_OK); - finish(); + } catch (AuthenticationFailedException afe) { Log.e(K9.LOG_TAG, "Error while testing settings", afe); showErrorDialog( @@ -470,6 +449,44 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList return null; } + private boolean cancelled() { + if (mDestroyed) { + return true; + } + if (mCanceled) { + finish(); + return true; + } + return false; + } + + private void checkOutgoing() throws MessagingException { + if (!(account.getRemoteStore() instanceof WebDavStore)) { + publishProgress(R.string.account_setup_check_settings_check_outgoing_msg); + } + Transport transport = Transport.getInstance(K9.app, account); + transport.close(); + transport.open(); + transport.close(); + } + + private void checkIncoming() throws MessagingException { + Store store = account.getRemoteStore(); + if (store instanceof WebDavStore) { + publishProgress(R.string.account_setup_check_settings_authenticate); + } else { + publishProgress(R.string.account_setup_check_settings_check_incoming_msg); + } + store.checkSettings(); + + if (store instanceof WebDavStore) { + publishProgress(R.string.account_setup_check_settings_fetch); + } + MessagingController.getInstance(getApplication()).listFoldersSynchronous(account, true, null); + MessagingController.getInstance(getApplication()) + .synchronizeMailbox(account, account.getInboxFolderName(), null, null); + } + @Override protected void onProgressUpdate(Integer... values) { setMessage(values[0]); From d0fa82269ff124a35de0b9755641c6e4859424ae Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Mon, 23 Feb 2015 17:28:42 +0000 Subject: [PATCH 2/2] review comments --- .../setup/AccountSetupCheckSettings.java | 45 ++++++++++++------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index 99c535abf..6073a98f6 100644 --- a/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/k9mail/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -392,6 +392,10 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList } } + /** + * FIXME: Don't use an AsyncTask to perform network operations. + * See also discussion in https://github.com/k9mail/k-9/pull/560 + */ private class CheckAccountTask extends AsyncTask { private final Account account; @@ -410,28 +414,18 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList */ if (cancelled()) { return null; - } else { - final MessagingController ctrl = MessagingController.getInstance(getApplication()); - ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, - account, direction); - - if (direction == CheckDirection.INCOMING) { - checkIncoming(); - } } + clearCertificateErrorNotifications(direction); + + checkServerSettings(direction); + if (cancelled()) { return null; - } else if (direction == CheckDirection.OUTGOING) { - checkOutgoing(); } - if (cancelled()) { - return null; - } else { - setResult(RESULT_OK); - finish(); - } + setResult(RESULT_OK); + finish(); } catch (AuthenticationFailedException afe) { Log.e(K9.LOG_TAG, "Error while testing settings", afe); @@ -449,6 +443,12 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList return null; } + private void clearCertificateErrorNotifications(CheckDirection direction) { + final MessagingController ctrl = MessagingController.getInstance(getApplication()); + ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, + account, direction); + } + private boolean cancelled() { if (mDestroyed) { return true; @@ -460,6 +460,19 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList return false; } + private void checkServerSettings(CheckDirection direction) throws MessagingException { + switch (direction) { + case INCOMING: { + checkIncoming(); + break; + } + case OUTGOING: { + checkOutgoing(); + break; + } + } + } + private void checkOutgoing() throws MessagingException { if (!(account.getRemoteStore() instanceof WebDavStore)) { publishProgress(R.string.account_setup_check_settings_check_outgoing_msg);