From dd1ec5f47b2e2ccb5caaa575393db7494e3f0c01 Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sat, 11 Oct 2014 11:54:26 +0100 Subject: [PATCH 1/5] add unit test --- .../src/com/fsck/k9/helper/UtilityTest.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java diff --git a/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java b/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java new file mode 100644 index 000000000..0f0e7baa3 --- /dev/null +++ b/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java @@ -0,0 +1,28 @@ +package com.fsck.k9.mail.internet; + +import com.fsck.k9.helper.Utility; + +import junit.framework.TestCase; + +import java.io.Serializable; +import java.util.LinkedList; + +public class UtilityTest extends TestCase { + + public void testToSerializableList() { + LinkedList input = new LinkedList(Arrays.asList("a", "b")); + + Serializable serializableList = Utility.toSerializableList(input); + + assertEquals(serializableList, input); + } + + public void testToSerializableListAlreadySerializable() { + ArrayList input = new ArrayList(Arrays.asList("a", "b")); + + Serializable serializableList = Utility.toSerializableList(input); + + assertSame(serializableList, input); + } + +} From c6df8f1ba1227cb0d400ca26fa5c2da0919286e9 Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sat, 11 Oct 2014 12:37:36 +0100 Subject: [PATCH 2/5] warn if not serializable, also add basic unit test --- src/com/fsck/k9/helper/Utility.java | 26 ++++++++++++++----- .../src/com/fsck/k9/helper/UtilityTest.java | 12 ++++----- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/com/fsck/k9/helper/Utility.java b/src/com/fsck/k9/helper/Utility.java index c450ad448..e97e852ef 100644 --- a/src/com/fsck/k9/helper/Utility.java +++ b/src/com/fsck/k9/helper/Utility.java @@ -709,21 +709,33 @@ public class Utility { } public static Serializable toSerializableList(List list) { - return list instanceof Serializable ? + return isExpectedCollectionClass(Serializable.class, list) ? (Serializable) list : new ArrayList(list); } public static ArrayList toArrayList(List list) { - return list instanceof ArrayList ? + return isExpectedCollectionClass(ArrayList.class, list) ? (ArrayList) list : new ArrayList(list); } - - public static Serializable toSerializableConcurrentMap(ConcurrentMap list) { - return list instanceof ConcurrentHashMap ? - (ConcurrentHashMap) list : - new ConcurrentHashMap(list); + public static Serializable toSerializableConcurrentMap(ConcurrentMap map) { + return isExpectedCollectionClass(ConcurrentHashMap.class, map) ? + (ConcurrentHashMap) map : + new ConcurrentHashMap(map); } + + private static boolean isExpectedCollectionClass(Class expected, Object instance) { +// Objects.requireNonNull(instance); // uncomment when project moves to API level 19 + if (!expected.isInstance(instance)) { + Log.w(K9.LOG_TAG, "Instance class [" + instance.getClass().getName() + + "] would be better as [" + expected.getName() + + "] for performance, to prevent collection copying"); + return false; + } else { + return true; + } + } + } diff --git a/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java b/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java index 0f0e7baa3..a0fff4d2e 100644 --- a/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java +++ b/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java @@ -4,23 +4,23 @@ import com.fsck.k9.helper.Utility; import junit.framework.TestCase; -import java.io.Serializable; -import java.util.LinkedList; +import java.io.*; +import java.util.*; public class UtilityTest extends TestCase { - public void testToSerializableList() { + public void testToArrayList() { LinkedList input = new LinkedList(Arrays.asList("a", "b")); - Serializable serializableList = Utility.toSerializableList(input); + Serializable serializableList = Utility.toArrayList(input); assertEquals(serializableList, input); } - public void testToSerializableListAlreadySerializable() { + public void testToArrayListAlreadyArrayList() { ArrayList input = new ArrayList(Arrays.asList("a", "b")); - Serializable serializableList = Utility.toSerializableList(input); + Serializable serializableList = Utility.toArrayList(input); assertSame(serializableList, input); } From 5dc1b82340f71a7a8e2cfcf396cb5fe5e857717d Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sat, 11 Oct 2014 23:52:48 +0100 Subject: [PATCH 3/5] address review comments --- src/com/fsck/k9/activity/MessageCompose.java | 4 +-- .../NotificationDeleteConfirmation.java | 3 ++ src/com/fsck/k9/helper/Utility.java | 31 +++++-------------- .../k9/service/NotificationActionService.java | 6 ++++ .../src/com/fsck/k9/helper/UtilityTest.java | 28 ----------------- 5 files changed, 18 insertions(+), 54 deletions(-) delete mode 100644 tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java diff --git a/src/com/fsck/k9/activity/MessageCompose.java b/src/com/fsck/k9/activity/MessageCompose.java index 4a0ab4cfa..9a9a6c16e 100644 --- a/src/com/fsck/k9/activity/MessageCompose.java +++ b/src/com/fsck/k9/activity/MessageCompose.java @@ -1155,7 +1155,7 @@ public class MessageCompose extends K9Activity implements OnClickListener, @Override protected void onSaveInstanceState(Bundle outState) { super.onSaveInstanceState(outState); - List attachments = new ArrayList(); + ArrayList attachments = new ArrayList(); for (int i = 0, count = mAttachments.getChildCount(); i < count; i++) { View view = mAttachments.getChildAt(i); Attachment attachment = (Attachment) view.getTag(); @@ -1164,7 +1164,7 @@ public class MessageCompose extends K9Activity implements OnClickListener, outState.putInt(STATE_KEY_NUM_ATTACHMENTS_LOADING, mNumAttachmentsLoading); outState.putString(STATE_KEY_WAITING_FOR_ATTACHMENTS, mWaitingForAttachments.name()); - outState.putParcelableArrayList(STATE_KEY_ATTACHMENTS, Utility.toArrayList(attachments)); + outState.putParcelableArrayList(STATE_KEY_ATTACHMENTS, attachments); outState.putBoolean(STATE_KEY_CC_SHOWN, mCcWrapper.getVisibility() == View.VISIBLE); outState.putBoolean(STATE_KEY_BCC_SHOWN, mBccWrapper.getVisibility() == View.VISIBLE); outState.putSerializable(STATE_KEY_QUOTED_TEXT_MODE, mQuotedTextMode); diff --git a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java index 9a070ed39..bd234d0e7 100644 --- a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java +++ b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java @@ -27,6 +27,9 @@ public class NotificationDeleteConfirmation extends Activity { private Account mAccount; private List mMessageRefs; + /** + * @param refs must be Serializable + */ public static PendingIntent getIntent(Context context, final Account account, final List refs) { Intent i = new Intent(context, NotificationDeleteConfirmation.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); diff --git a/src/com/fsck/k9/helper/Utility.java b/src/com/fsck/k9/helper/Utility.java index e97e852ef..75150a147 100644 --- a/src/com/fsck/k9/helper/Utility.java +++ b/src/com/fsck/k9/helper/Utility.java @@ -708,34 +708,17 @@ public class Utility { return sMainThreadHandler; } + /** + * @return the supplied list casted to Serializable. + * + * See https://github.com/k9mail/k9mail_pgp_mime/pull/12#issuecomment-58767082 + */ public static Serializable toSerializableList(List list) { - return isExpectedCollectionClass(Serializable.class, list) ? - (Serializable) list : - new ArrayList(list); - } - - public static ArrayList toArrayList(List list) { - return isExpectedCollectionClass(ArrayList.class, list) ? - (ArrayList) list : - new ArrayList(list); + return (Serializable) list; } public static Serializable toSerializableConcurrentMap(ConcurrentMap map) { - return isExpectedCollectionClass(ConcurrentHashMap.class, map) ? - (ConcurrentHashMap) map : - new ConcurrentHashMap(map); - } - - private static boolean isExpectedCollectionClass(Class expected, Object instance) { -// Objects.requireNonNull(instance); // uncomment when project moves to API level 19 - if (!expected.isInstance(instance)) { - Log.w(K9.LOG_TAG, "Instance class [" + instance.getClass().getName() - + "] would be better as [" + expected.getName() - + "] for performance, to prevent collection copying"); - return false; - } else { - return true; - } + return (ConcurrentHashMap) map; } } diff --git a/src/com/fsck/k9/service/NotificationActionService.java b/src/com/fsck/k9/service/NotificationActionService.java index e4cad21c9..a29b50568 100644 --- a/src/com/fsck/k9/service/NotificationActionService.java +++ b/src/com/fsck/k9/service/NotificationActionService.java @@ -37,6 +37,9 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } + /** + * @param refs must be Serializable + */ public static PendingIntent getReadAllMessagesIntent(Context context, final Account account, final List refs) { Intent i = new Intent(context, NotificationActionService.class); @@ -55,6 +58,9 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } + /** + * @param refs must be Serializable + */ public static Intent getDeleteAllMessagesIntent(Context context, final Account account, final List refs) { Intent i = new Intent(context, NotificationActionService.class); diff --git a/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java b/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java deleted file mode 100644 index a0fff4d2e..000000000 --- a/tests-on-jvm/src/com/fsck/k9/helper/UtilityTest.java +++ /dev/null @@ -1,28 +0,0 @@ -package com.fsck.k9.mail.internet; - -import com.fsck.k9.helper.Utility; - -import junit.framework.TestCase; - -import java.io.*; -import java.util.*; - -public class UtilityTest extends TestCase { - - public void testToArrayList() { - LinkedList input = new LinkedList(Arrays.asList("a", "b")); - - Serializable serializableList = Utility.toArrayList(input); - - assertEquals(serializableList, input); - } - - public void testToArrayListAlreadyArrayList() { - ArrayList input = new ArrayList(Arrays.asList("a", "b")); - - Serializable serializableList = Utility.toArrayList(input); - - assertSame(serializableList, input); - } - -} From ba26cfce90630d88ccb524d00addb22ac6e95cfe Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sun, 12 Oct 2014 08:54:44 +0100 Subject: [PATCH 4/5] remove controversial methods --- src/com/fsck/k9/activity/Accounts.java | 7 +++++-- .../NotificationDeleteConfirmation.java | 10 ++++------ .../k9/controller/MessagingController.java | 13 ++++++------- src/com/fsck/k9/helper/Utility.java | 13 ------------- .../k9/service/NotificationActionService.java | 19 +++++++------------ 5 files changed, 22 insertions(+), 40 deletions(-) diff --git a/src/com/fsck/k9/activity/Accounts.java b/src/com/fsck/k9/activity/Accounts.java index 0b9a76f06..91e01ef19 100644 --- a/src/com/fsck/k9/activity/Accounts.java +++ b/src/com/fsck/k9/activity/Accounts.java @@ -122,7 +122,10 @@ public class Accounts extends K9ListActivity implements OnItemClickListener { private static final int DIALOG_RECREATE_ACCOUNT = 3; private static final int DIALOG_NO_FILE_MANAGER = 4; - private ConcurrentMap accountStats = new ConcurrentHashMap(); + /* + * Must be serializable hence implementation class used for declaration. + */ + private ConcurrentHashMap accountStats = new ConcurrentHashMap(); private ConcurrentMap pendingWork = new ConcurrentHashMap(); @@ -497,7 +500,7 @@ public class Accounts extends K9ListActivity implements OnItemClickListener { outState.putString(SELECTED_CONTEXT_ACCOUNT, mSelectedContextAccount.getUuid()); } outState.putSerializable(STATE_UNREAD_COUNT, mUnreadMessageCount); - outState.putSerializable(ACCOUNT_STATS, Utility.toSerializableConcurrentMap(accountStats)); + outState.putSerializable(ACCOUNT_STATS, accountStats); } private StorageManager.StorageListener storageListener = new StorageManager.StorageListener() { diff --git a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java index bd234d0e7..c3fd3e290 100644 --- a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java +++ b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java @@ -1,5 +1,6 @@ package com.fsck.k9.activity; +import java.io.Serializable; import java.util.ArrayList; import java.util.List; @@ -25,15 +26,12 @@ public class NotificationDeleteConfirmation extends Activity { private final static int DIALOG_CONFIRM = 1; private Account mAccount; - private List mMessageRefs; + private ArrayList mMessageRefs; - /** - * @param refs must be Serializable - */ - public static PendingIntent getIntent(Context context, final Account account, final List refs) { + public static & Serializable> PendingIntent getIntent(Context context, final Account account, final T refs) { Intent i = new Intent(context, NotificationDeleteConfirmation.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - i.putExtra(EXTRA_MESSAGE_LIST, Utility.toSerializableList(refs)); + i.putExtra(EXTRA_MESSAGE_LIST, refs); i.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TOP); return PendingIntent.getActivity(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index 49ecfea36..f05fc690b 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -2,6 +2,7 @@ package com.fsck.k9.controller; import java.io.CharArrayWriter; import java.io.PrintWriter; +import java.io.Serializable; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -295,17 +296,14 @@ public class MessagingController implements Runnable { } /** - * Gets a list of references for all pending messages for the notification. - * - * @return Message reference list + * Adds a list of references for all pending messages for the notification to the supplied + * List. */ - public List getAllMessageRefs() { - List refs = new ArrayList(); + public void supplyAllMessageRefs(List refs) { for (Message m : messages) { refs.add(m.makeMessageReference()); } refs.addAll(droppedMessages); - return refs; } /** @@ -4863,7 +4861,8 @@ public class MessagingController implements Runnable { String accountDescr = (account.getDescription() != null) ? account.getDescription() : account.getEmail(); - final List allRefs = data.getAllMessageRefs(); + final ArrayList allRefs = new ArrayList(); + data.supplyAllMessageRefs(allRefs); if (platformSupportsExtendedNotifications() && !privacyModeEnabled) { if (newMessages > 1) { diff --git a/src/com/fsck/k9/helper/Utility.java b/src/com/fsck/k9/helper/Utility.java index 75150a147..abad5f94f 100644 --- a/src/com/fsck/k9/helper/Utility.java +++ b/src/com/fsck/k9/helper/Utility.java @@ -708,17 +708,4 @@ public class Utility { return sMainThreadHandler; } - /** - * @return the supplied list casted to Serializable. - * - * See https://github.com/k9mail/k9mail_pgp_mime/pull/12#issuecomment-58767082 - */ - public static Serializable toSerializableList(List list) { - return (Serializable) list; - } - - public static Serializable toSerializableConcurrentMap(ConcurrentMap map) { - return (ConcurrentHashMap) map; - } - } diff --git a/src/com/fsck/k9/service/NotificationActionService.java b/src/com/fsck/k9/service/NotificationActionService.java index a29b50568..7d6cbd5b4 100644 --- a/src/com/fsck/k9/service/NotificationActionService.java +++ b/src/com/fsck/k9/service/NotificationActionService.java @@ -1,5 +1,6 @@ package com.fsck.k9.service; +import java.io.Serializable; import java.util.ArrayList; import java.util.List; @@ -37,14 +38,11 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } - /** - * @param refs must be Serializable - */ - public static PendingIntent getReadAllMessagesIntent(Context context, final Account account, - final List refs) { + public static & Serializable> PendingIntent getReadAllMessagesIntent(Context context, final Account account, + final T refs) { Intent i = new Intent(context, NotificationActionService.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - i.putExtra(EXTRA_MESSAGE_LIST, Utility.toSerializableList(refs)); + i.putExtra(EXTRA_MESSAGE_LIST, refs); i.setAction(READ_ALL_ACTION); return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); @@ -58,14 +56,11 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } - /** - * @param refs must be Serializable - */ - public static Intent getDeleteAllMessagesIntent(Context context, final Account account, - final List refs) { + public static & Serializable> Intent getDeleteAllMessagesIntent(Context context, final Account account, + final T refs) { Intent i = new Intent(context, NotificationActionService.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - i.putExtra(EXTRA_MESSAGE_LIST, Utility.toSerializableList(refs)); + i.putExtra(EXTRA_MESSAGE_LIST, refs); i.setAction(DELETE_ALL_ACTION); return i; From a9b0907c312bd40ca7535681cc5c5f0e2f60481f Mon Sep 17 00:00:00 2001 From: Art O Cathain Date: Sun, 12 Oct 2014 09:24:08 +0100 Subject: [PATCH 5/5] further simplification --- .../fsck/k9/activity/NotificationDeleteConfirmation.java | 2 +- src/com/fsck/k9/service/NotificationActionService.java | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java index c3fd3e290..fd6f87db5 100644 --- a/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java +++ b/src/com/fsck/k9/activity/NotificationDeleteConfirmation.java @@ -28,7 +28,7 @@ public class NotificationDeleteConfirmation extends Activity { private Account mAccount; private ArrayList mMessageRefs; - public static & Serializable> PendingIntent getIntent(Context context, final Account account, final T refs) { + public static PendingIntent getIntent(Context context, final Account account, final Serializable refs) { Intent i = new Intent(context, NotificationDeleteConfirmation.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); i.putExtra(EXTRA_MESSAGE_LIST, refs); diff --git a/src/com/fsck/k9/service/NotificationActionService.java b/src/com/fsck/k9/service/NotificationActionService.java index 7d6cbd5b4..107bd0f4c 100644 --- a/src/com/fsck/k9/service/NotificationActionService.java +++ b/src/com/fsck/k9/service/NotificationActionService.java @@ -38,8 +38,7 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } - public static & Serializable> PendingIntent getReadAllMessagesIntent(Context context, final Account account, - final T refs) { + public static PendingIntent getReadAllMessagesIntent(Context context, final Account account, final Serializable refs) { Intent i = new Intent(context, NotificationActionService.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); i.putExtra(EXTRA_MESSAGE_LIST, refs); @@ -56,8 +55,7 @@ public class NotificationActionService extends CoreService { return PendingIntent.getService(context, account.getAccountNumber(), i, PendingIntent.FLAG_UPDATE_CURRENT); } - public static & Serializable> Intent getDeleteAllMessagesIntent(Context context, final Account account, - final T refs) { + public static Intent getDeleteAllMessagesIntent(Context context, final Account account, final Serializable refs) { Intent i = new Intent(context, NotificationActionService.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); i.putExtra(EXTRA_MESSAGE_LIST, refs);