From 84a5e34537b32fddd29d36ae54b9d6651ac7e4da Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Tue, 8 Jan 2013 09:27:28 +0100 Subject: [PATCH 1/4] Improve notification message list processing - If a message contained in the inbox list is deleted or read while there are messages in the overflow list, restore the newest message from the overflow list so there are always 5 messages in the inbox list in that case. - Use explicit methods instead of method overriding. --- .../k9/controller/MessagingController.java | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index a6ebbece1..480a21fa5 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -201,29 +201,36 @@ public class MessagingController implements Runnable { private ConcurrentHashMap deletedUids = new ConcurrentHashMap(); private static class NotificationData { - int unreadBeforeNotification; - LinkedList messages; // newest one first - LinkedList droppedMessages; // newest one first + private int unreadBeforeNotification; + private LinkedList messages; // newest one first + private LinkedList droppedMessages; // newest one first // There's no point in storing more than 5 messages for the notification, as a single notification // can't display more than that anyway. private final static int MAX_MESSAGES = 5; - @SuppressWarnings("serial") public NotificationData(int unread) { unreadBeforeNotification = unread; droppedMessages = new LinkedList(); - messages = new LinkedList() { - @Override - public boolean add(Message m) { - while (size() >= MAX_MESSAGES) { - Message dropped = super.removeLast(); - droppedMessages.add(0, dropped.makeMessageReference()); - } - super.add(0, m); - return true; + messages = new LinkedList(); + } + + public void addMessage(Message m) { + while (messages.size() >= MAX_MESSAGES) { + Message dropped = messages.removeLast(); + droppedMessages.addFirst(dropped.makeMessageReference()); + } + messages.addFirst(m); + } + + public void removeMessage(Context context, Message m) { + if (messages.remove(m) && !droppedMessages.isEmpty()) { + Message message = droppedMessages.getFirst().restoreToLocalMessage(context); + if (message != null) { + messages.addLast(message); + droppedMessages.removeFirst(); } - }; + } } public ArrayList getAllMessageRefs() { @@ -1756,7 +1763,7 @@ public class MessagingController implements Runnable { for (Message m : data.messages) { if (uid.equals(m.getUid()) && !shouldBeNotifiedOf) { - data.messages.remove(m); + data.removeMessage(mApplication, m); needUpdateNotification = true; break; } @@ -1764,7 +1771,7 @@ public class MessagingController implements Runnable { if (!needUpdateNotification) { for (MessageReference dropped : data.droppedMessages) { if (uid.equals(dropped.uid) && !shouldBeNotifiedOf) { - data.droppedMessages.remove(dropped.uid); + data.droppedMessages.remove(dropped); needUpdateNotification = true; break; } @@ -4568,11 +4575,11 @@ public class MessagingController implements Runnable { private Message findNewestMessageForNotificationLocked(Context context, Account account, NotificationData data) { if (!data.messages.isEmpty()) { - return data.messages.get(0); + return data.messages.getFirst(); } if (!data.droppedMessages.isEmpty()) { - return data.droppedMessages.get(0).restoreToLocalMessage(context); + return data.droppedMessages.getFirst().restoreToLocalMessage(context); } return null; @@ -4601,7 +4608,7 @@ public class MessagingController implements Runnable { return; } } else { - data.messages.add(message); + data.addMessage(message); } final KeyguardManager keyguardService = (KeyguardManager) context.getSystemService(Context.KEYGUARD_SERVICE); From cb9cff382f6a431870e955be5ef7ef82b529c320 Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Tue, 8 Jan 2013 09:29:34 +0100 Subject: [PATCH 2/4] Fix typo causing a NPE. --- src/com/fsck/k9/activity/MessageView.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/com/fsck/k9/activity/MessageView.java b/src/com/fsck/k9/activity/MessageView.java index 1e3e663bf..3ebd01ed4 100644 --- a/src/com/fsck/k9/activity/MessageView.java +++ b/src/com/fsck/k9/activity/MessageView.java @@ -447,7 +447,7 @@ public class MessageView extends K9FragmentActivity implements MessageViewFragme private void showNextMessage() { findSurroundingMessagesUid(); - if (mMessageReferences == null) { + if (mMessageReferences != null) { mMessageReferences.remove(mMessageReference); } if (mLastDirection == NEXT && mNextMessage != null) { From 4d075c91ac4f7d3f1162109229dc94750ca12a8b Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Tue, 8 Jan 2013 10:06:01 +0100 Subject: [PATCH 3/4] Some more cleanup - When the last message of the message list is cleared, clear the whole notification - Compare whole message reference, not only UID. --- .../k9/controller/MessagingController.java | 60 ++++++++++--------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index 480a21fa5..ae48ab83b 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -223,14 +223,28 @@ public class MessagingController implements Runnable { messages.addFirst(m); } - public void removeMessage(Context context, Message m) { - if (messages.remove(m) && !droppedMessages.isEmpty()) { - Message message = droppedMessages.getFirst().restoreToLocalMessage(context); - if (message != null) { - messages.addLast(message); - droppedMessages.removeFirst(); + public boolean removeMatchingMessage(Context context, MessageReference ref) { + for (MessageReference dropped : droppedMessages) { + if (dropped.equals(ref)) { + droppedMessages.remove(dropped); + return true; } } + + for (Message message : messages) { + if (message.makeMessageReference().equals(ref)) { + if (messages.remove(message) && !droppedMessages.isEmpty()) { + Message restoredMessage = droppedMessages.getFirst().restoreToLocalMessage(context); + if (restoredMessage != null) { + messages.addLast(restoredMessage); + droppedMessages.removeFirst(); + } + } + return true; + } + } + + return false; } public ArrayList getAllMessageRefs() { @@ -1755,31 +1769,16 @@ public class MessagingController implements Runnable { } } - NotificationData data = getNotificationData(account, -1); - if (data != null) { - synchronized (data) { - boolean needUpdateNotification = false; - String uid = localMessage.getUid(); - - for (Message m : data.messages) { - if (uid.equals(m.getUid()) && !shouldBeNotifiedOf) { - data.removeMessage(mApplication, m); - needUpdateNotification = true; - break; + // we're only interested in messages that need removing + if (!shouldBeNotifiedOf) { + NotificationData data = getNotificationData(account, -1); + if (data != null) { + synchronized (data) { + MessageReference ref = localMessage.makeMessageReference(); + if (data.removeMatchingMessage(mApplication, ref)) { + notifyAccountWithDataLocked(mApplication, account, null, data); } } - if (!needUpdateNotification) { - for (MessageReference dropped : data.droppedMessages) { - if (uid.equals(dropped.uid) && !shouldBeNotifiedOf) { - data.droppedMessages.remove(dropped); - needUpdateNotification = true; - break; - } - } - } - if (needUpdateNotification) { - notifyAccountWithDataLocked(mApplication, account, null, data); - } } } } @@ -4605,6 +4604,9 @@ public class MessagingController implements Runnable { message = findNewestMessageForNotificationLocked(context, account, data); updateSilently = true; if (message == null) { + // seemingly both the message list as well as the overflow list is empty; + // it probably is a good idea to cancel the notification in that case + notifyAccountCancel(context, account); return; } } else { From ada2a9ccb55a3f14ced71b3fd4f05408adabc02a Mon Sep 17 00:00:00 2001 From: Danny Baumann Date: Tue, 8 Jan 2013 12:52:56 +0100 Subject: [PATCH 4/4] Documentation and cleanup - Add Javadoc to new methods and classes - Get rid of magic number --- .../k9/controller/MessagingController.java | 79 +++++++++++++++++-- 1 file changed, 71 insertions(+), 8 deletions(-) diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index ae48ab83b..6ff3550b3 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -200,21 +200,56 @@ public class MessagingController implements Runnable { // Key is accountUuid:folderName:messageUid , value is unimportant private ConcurrentHashMap deletedUids = new ConcurrentHashMap(); + /** + * A holder class for pending notification data + * + * This class holds all pieces of information for constructing + * a notification with message preview. + */ private static class NotificationData { - private int unreadBeforeNotification; - private LinkedList messages; // newest one first - private LinkedList droppedMessages; // newest one first + /** Number of unread messages before constructing the notification */ + int unreadBeforeNotification; + /** + * List of messages that should be used for the inbox-style overview. + * It's sorted from newest to oldest message. + * Don't modify this list directly, but use {@link addMessage} and + * {@link removeMatchingMessage} instead. + */ + LinkedList messages; + /** + * List of references for messages that the user is still to be notified of, + * but which don't fit into the inbox style anymore. It's sorted from newest + * to oldest message. + */ + LinkedList droppedMessages; - // There's no point in storing more than 5 messages for the notification, as a single notification - // can't display more than that anyway. + /** + * Maximum number of messages to keep for the inbox-style overview. + * As of Jellybean, phone notifications show a maximum of 5 lines, while tablet + * notifications show 7 lines. To make sure no lines are silently dropped, + * we default to 5 lines. + */ private final static int MAX_MESSAGES = 5; + /** + * Constructs a new data instance. + * + * @param unread Number of unread messages prior to instance construction + */ public NotificationData(int unread) { unreadBeforeNotification = unread; droppedMessages = new LinkedList(); messages = new LinkedList(); } + /** + * Adds a new message to the list of pending messages for this notification. + * + * The implementation will take care of keeping a meaningful amount of + * messages in {@link #messages}. + * + * @param m The new message to add. + */ public void addMessage(Message m) { while (messages.size() >= MAX_MESSAGES) { Message dropped = messages.removeLast(); @@ -223,6 +258,13 @@ public class MessagingController implements Runnable { messages.addFirst(m); } + /** + * Remove a certain message from the message list. + * + * @param context A context. + * @param ref Reference of the message to remove + * @return true if message was found and removed, false otherwise + */ public boolean removeMatchingMessage(Context context, MessageReference ref) { for (MessageReference dropped : droppedMessages) { if (dropped.equals(ref)) { @@ -247,6 +289,11 @@ public class MessagingController implements Runnable { return false; } + /** + * Gets a list of references for all pending messages for the notification. + * + * @return Message reference list + */ public ArrayList getAllMessageRefs() { ArrayList refs = new ArrayList(); for (Message m : messages) { @@ -256,6 +303,11 @@ public class MessagingController implements Runnable { return refs; } + /** + * Gets the total number of messages the user is to be notified of. + * + * @return Amount of new messages the notification notifies for + */ public int getNewMessageCount() { return messages.size() + droppedMessages.size(); } @@ -1771,7 +1823,7 @@ public class MessagingController implements Runnable { // we're only interested in messages that need removing if (!shouldBeNotifiedOf) { - NotificationData data = getNotificationData(account, -1); + NotificationData data = getNotificationData(account, null); if (data != null) { synchronized (data) { MessageReference ref = localMessage.makeMessageReference(); @@ -4461,12 +4513,23 @@ public class MessagingController implements Runnable { return true; } - private NotificationData getNotificationData(Account account, int previousUnreadMessageCount) { + /** + * Get the pending notification data for an account. + * See {@link NotificationData}. + * + * @param account The account to retrieve the pending data for + * @param previousUnreadMessageCount The number of currently pending messages, which will be used + * if there's no pending data yet. If passed as null, a new instance + * won't be created if currently not existent. + * @return A pending data instance, or null if one doesn't exist and + * previousUnreadMessageCount was passed as null. + */ + private NotificationData getNotificationData(Account account, Integer previousUnreadMessageCount) { NotificationData data; synchronized (notificationData) { data = notificationData.get(account.getAccountNumber()); - if (data == null && previousUnreadMessageCount >= 0) { + if (data == null && previousUnreadMessageCount != null) { data = new NotificationData(previousUnreadMessageCount); notificationData.put(account.getAccountNumber(), data); }