Refactored message sorting code by extracting the sort code from the sorted object (this was a bad design, worsened by the MessageProvider patch). Ideally, new Comparator classes should get promoted to top-level classes and not be enclosed in MessageList.

Subject stripper backported from issue258 branch (Utility.java)
The result is a cleaner MessageInfoHolder class.

src/com/fsck/k9/activity/MessageInfoHolder.java

@@ -1,7 +1,5 @@
 package com.fsck.k9.activity;
 import java.util.Date;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 
 import android.content.Context;
 import android.text.SpannableStringBuilder;
@@ -11,19 +9,16 @@ import android.util.Log;
 import com.fsck.k9.Account;
 import com.fsck.k9.K9;
 import com.fsck.k9.R;
-import com.fsck.k9.activity.FolderInfoHolder;
-import com.fsck.k9.controller.MessagingController;
-import com.fsck.k9.controller.MessagingController.SORT_TYPE;
 import com.fsck.k9.helper.Contacts;
 import com.fsck.k9.helper.Utility;
 import com.fsck.k9.mail.Address;
 import com.fsck.k9.mail.Flag;
 import com.fsck.k9.mail.Message;
-import com.fsck.k9.mail.Message.RecipientType;
 import com.fsck.k9.mail.MessagingException;
+import com.fsck.k9.mail.Message.RecipientType;
 import com.fsck.k9.mail.store.LocalStore.LocalMessage;
 
-public class MessageInfoHolder implements Comparable<MessageInfoHolder>
+public class MessageInfoHolder
 {
     public String subject;
     public String date;
@@ -50,12 +45,6 @@ public class MessageInfoHolder implements Comparable<MessageInfoHolder>
 
     private Contacts mContacts;
 
-    private SORT_TYPE sortType = SORT_TYPE.SORT_DATE;
-
-    private boolean sortAscending = true;
-    private boolean sortDateAscending = false;
-    private MessagingController mController;
-
     // Empty constructor for comparison
     public MessageInfoHolder()
     {
@@ -69,24 +58,6 @@ public class MessageInfoHolder implements Comparable<MessageInfoHolder>
         mContacts = Contacts.getInstance(context);
 
         Account account = m.getFolder().getAccount();
-        mController = MessagingController.getInstance(K9.app);
-        sortType = mController.getSortType();
-        sortAscending = mController.isSortAscending(sortType);
-        sortDateAscending = mController.isSortAscending(SORT_TYPE.SORT_DATE);
-        populate(context, m, new FolderInfoHolder(context, m.getFolder(), m.getFolder().getAccount()), account);
-    }
-
-    public MessageInfoHolder(Context context ,Message m, SORT_TYPE t_sort, boolean asc)
-    {
-        this();
-
-        mContacts = Contacts.getInstance(context);
-
-        Account account = m.getFolder().getAccount();
-        mController = MessagingController.getInstance(K9.app);
-        sortType = t_sort;
-        sortAscending = asc;
-        sortDateAscending = asc;
         populate(context, m, new FolderInfoHolder(context, m.getFolder(), m.getFolder().getAccount()), account);
     }
 
@@ -96,12 +67,7 @@ public class MessageInfoHolder implements Comparable<MessageInfoHolder>
 
         mContacts = Contacts.getInstance(context);
 
-        mController = MessagingController.getInstance(K9.app);
-        sortType = mController.getSortType();
-        sortAscending = mController.isSortAscending(sortType);
-        sortDateAscending = mController.isSortAscending(SORT_TYPE.SORT_DATE);
         populate(context, m, folder, account);
-
     }
 
     public void populate(Context context, Message m, FolderInfoHolder folder, Account account)
@@ -196,80 +162,4 @@ public class MessageInfoHolder implements Comparable<MessageInfoHolder>
         return uid.hashCode();
     }
 
-    public int compareTo(MessageInfoHolder o)
-    {
-        int ascender = (sortAscending ? 1 : -1);
-        int comparison = 0;
-
-        if (sortType == SORT_TYPE.SORT_SUBJECT)
-        {
-            if (compareSubject == null)
-            {
-                compareSubject = stripPrefixes(subject).toLowerCase();
-            }
-
-            if (o.compareSubject == null)
-            {
-                o.compareSubject = stripPrefixes(o.subject).toLowerCase();
-            }
-
-            comparison = this.compareSubject.compareTo(o.compareSubject);
-        }
-        else if (sortType == SORT_TYPE.SORT_SENDER)
-        {
-            comparison = this.compareCounterparty.toLowerCase().compareTo(o.compareCounterparty.toLowerCase());
-        }
-        else if (sortType == SORT_TYPE.SORT_FLAGGED)
-        {
-            comparison = (this.flagged ? 0 : 1) - (o.flagged ? 0 : 1);
-        }
-        else if (sortType == SORT_TYPE.SORT_UNREAD)
-        {
-            comparison = (this.read ? 1 : 0) - (o.read ? 1 : 0);
-        }
-        else if (sortType == SORT_TYPE.SORT_ATTACHMENT)
-        {
-            comparison = (this.hasAttachments ? 0 : 1) - (o.hasAttachments ? 0 : 1);
-        }
-
-        if (comparison != 0)
-        {
-            return comparison * ascender;
-        }
-
-        int dateAscender = (sortDateAscending ? 1 : -1);
-
-        return this.compareDate.compareTo(o.compareDate) * dateAscender;
-    }
-
-    Pattern pattern = null;
-    String patternString = "^ *(re|aw|fw|fwd): *";
-    private String stripPrefixes(String in)
-    {
-        synchronized (patternString)
-        {
-            if (pattern == null)
-            {
-                pattern = Pattern.compile(patternString, Pattern.CASE_INSENSITIVE);
-            }
-        }
-
-        Matcher matcher = pattern.matcher(in);
-
-        int lastPrefix = -1;
-
-        while (matcher.find())
-        {
-            lastPrefix = matcher.end();
-        }
-
-        if (lastPrefix > -1 && lastPrefix < in.length() - 1)
-        {
-            return in.substring(lastPrefix);
-        }
-        else
-        {
-            return in;
-        }
-    }
 }

src/com/fsck/k9/activity/MessageList.java

@@ -2,10 +2,10 @@ package com.fsck.k9.activity;
 
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Date;
+import java.util.Comparator;
+import java.util.EnumMap;
 import java.util.List;
-import java.util.regex.Matcher;
+import java.util.Map;
-import java.util.regex.Pattern;
 
 import android.app.AlertDialog;
 import android.app.Dialog;
@@ -20,34 +20,32 @@ import android.text.Spannable;
 import android.text.SpannableStringBuilder;
 import android.text.style.ForegroundColorSpan;
 import android.text.style.StyleSpan;
-import android.util.Config;
 import android.util.Log;
 import android.util.TypedValue;
 import android.view.ContextMenu;
-import android.view.ContextMenu.ContextMenuInfo;
 import android.view.GestureDetector;
-import android.view.GestureDetector.SimpleOnGestureListener;
 import android.view.KeyEvent;
 import android.view.LayoutInflater;
 import android.view.Menu;
 import android.view.MenuItem;
 import android.view.MotionEvent;
 import android.view.View;
-import android.view.View.OnClickListener;
 import android.view.ViewGroup;
 import android.view.Window;
+import android.view.ContextMenu.ContextMenuInfo;
+import android.view.GestureDetector.SimpleOnGestureListener;
+import android.view.View.OnClickListener;
 import android.widget.AdapterView;
-import android.widget.AdapterView.AdapterContextMenuInfo;
 import android.widget.BaseAdapter;
 import android.widget.CheckBox;
 import android.widget.CompoundButton;
-import android.widget.CompoundButton.OnCheckedChangeListener;
 import android.widget.ImageButton;
 import android.widget.ListView;
 import android.widget.ProgressBar;
 import android.widget.TextView;
 import android.widget.Toast;
-import android.text.format.DateFormat;
+import android.widget.AdapterView.AdapterContextMenuInfo;
+import android.widget.CompoundButton.OnCheckedChangeListener;
 
 import com.fsck.k9.Account;
 import com.fsck.k9.AccountStats;
@@ -60,21 +58,14 @@ import com.fsck.k9.activity.setup.AccountSettings;
 import com.fsck.k9.activity.setup.FolderSettings;
 import com.fsck.k9.activity.setup.Prefs;
 import com.fsck.k9.controller.MessagingController;
-import com.fsck.k9.controller.MessagingController.SORT_TYPE;
 import com.fsck.k9.controller.MessagingListener;
-import com.fsck.k9.helper.Contacts;
+import com.fsck.k9.controller.MessagingController.SORT_TYPE;
 import com.fsck.k9.helper.Utility;
-import com.fsck.k9.mail.Address;
 import com.fsck.k9.mail.Flag;
 import com.fsck.k9.mail.Folder;
 import com.fsck.k9.mail.Message;
-import com.fsck.k9.mail.Message.RecipientType;
-import com.fsck.k9.mail.MessagingException;
 import com.fsck.k9.mail.store.LocalStore;
 import com.fsck.k9.mail.store.LocalStore.LocalFolder;
-import com.fsck.k9.mail.store.LocalStore.LocalMessage;
-import com.fsck.k9.activity.MessageInfoHolder;
-import com.fsck.k9.activity.FolderInfoHolder;
 
 /**
  * MessageList is the primary user interface for the program. This Activity
@@ -86,6 +77,144 @@ public class MessageList
         implements OnClickListener, AdapterView.OnItemClickListener
 {
 
+    /**
+     * Reverses the result of a {@link Comparator}.
+     * 
+     * @param <T>
+     */
+    public static class ReverseComparator<T> implements Comparator<T>
+    {
+        private Comparator<T> mDelegate;
+
+        /**
+         * @param delegate
+         *            Never <code>null</code>.
+         */
+        public ReverseComparator(final Comparator<T> delegate)
+        {
+            mDelegate = delegate;
+        }
+
+        @Override
+        public int compare(final T object1, final T object2)
+        {
+            // arg1 & 2 are mixed up, this is done on purpose
+            return mDelegate.compare(object2, object1);
+        }
+
+    }
+
+    /**
+     * Chains comparator to find a non-0 result.
+     * 
+     * @param <T>
+     */
+    public static class ComparatorChain<T> implements Comparator<T>
+    {
+
+        private List<Comparator<T>> mChain;
+
+        /**
+         * @param chain
+         *            Comparator chain. Never <code>null</code>.
+         */
+        public ComparatorChain(final List<Comparator<T>> chain)
+        {
+            mChain = chain;
+        }
+
+        @Override
+        public int compare(T object1, T object2)
+        {
+            int result = 0;
+            for (final Comparator<T> comparator : mChain)
+            {
+                result = comparator.compare(object1, object2);
+                if (result != 0)
+                {
+                    break;
+                }
+            }
+            return result;
+        }
+        
+    }
+
+    public static class AttachmentComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder object1, MessageInfoHolder object2)
+        {
+            return (object1.hasAttachments ? 0 : 1) - (object2.hasAttachments ? 0 : 1);
+        }
+
+    }
+
+    public static class FlaggedComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder object1, MessageInfoHolder object2)
+        {
+            return (object1.flagged ? 0 : 1) - (object2.flagged ? 0 : 1);
+        }
+
+    }
+
+    public static class UnreadComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder object1, MessageInfoHolder object2)
+        {
+            return (object1.read ? 1 : 0) - (object2.read ? 1 : 0);
+        }
+
+    }
+
+    public static class SenderComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder object1, MessageInfoHolder object2)
+        {
+            return object1.compareCounterparty.toLowerCase().compareTo(object2.compareCounterparty.toLowerCase());
+        }
+
+    }
+
+    public static class DateComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder object1, MessageInfoHolder object2)
+        {
+            return object1.compareDate.compareTo(object2.compareDate);
+        }
+
+    }
+
+    public static class SubjectComparator implements Comparator<MessageInfoHolder>
+    {
+
+        @Override
+        public int compare(MessageInfoHolder arg0, MessageInfoHolder arg1)
+        {
+            // XXX doesn't respect the Comparator contract since it alters the compared object
+            if (arg0.compareSubject == null)
+            {
+                arg0.compareSubject = Utility.stripSubject(arg0.subject);
+            }
+            if (arg1.compareSubject == null)
+            {
+                arg1.compareSubject = Utility.stripSubject(arg1.subject);
+            }
+            return arg0.compareSubject.compareToIgnoreCase(arg1.compareSubject);
+        }
+
+    }
+
     /**
      * Immutable empty {@link Message} array
      */
@@ -109,6 +238,27 @@ public class MessageList
     private static final String EXTRA_TITLE = "title";
     private static final String EXTRA_LIST_POSITION = "listPosition";
 
+    /**
+     * Maps a {@link SORT_TYPE} to a {@link Comparator} implementation.
+     */
+    private static final Map<SORT_TYPE, Comparator<MessageInfoHolder>> SORT_COMPARATORS;
+
+    static
+    {
+        // fill the mapping at class time loading
+
+        final Map<SORT_TYPE, Comparator<MessageInfoHolder>> map = new EnumMap<SORT_TYPE, Comparator<MessageInfoHolder>>(SORT_TYPE.class);
+        map.put(SORT_TYPE.SORT_ATTACHMENT, new AttachmentComparator());
+        map.put(SORT_TYPE.SORT_DATE, new DateComparator());
+        map.put(SORT_TYPE.SORT_FLAGGED, new FlaggedComparator());
+        map.put(SORT_TYPE.SORT_SENDER, new SenderComparator());
+        map.put(SORT_TYPE.SORT_SUBJECT, new SubjectComparator());
+        map.put(SORT_TYPE.SORT_UNREAD, new UnreadComparator());
+
+        // make it immutable to prevent accidental alteration (content is immutable already)
+        SORT_COMPARATORS = Collections.unmodifiableMap(map);
+    }
+
     private ListView mListView;
 
     private boolean mTouchView = true;
@@ -211,7 +361,7 @@ public class MessageList
                             int index;
                             synchronized (mAdapter.messages)
                             {
-                                index = Collections.binarySearch(mAdapter.messages, message);
+                                index = Collections.binarySearch(mAdapter.messages, message, getComparator());
                             }
 
                             if (index < 0)
@@ -264,19 +414,64 @@ public class MessageList
 
         private void sortMessages()
         {
+            final Comparator<MessageInfoHolder> chainComparator = getComparator();
+
             runOnUiThread(new Runnable()
             {
                 public void run()
                 {
                     synchronized (mAdapter.messages)
                     {
-                        Collections.sort(mAdapter.messages);
+                        Collections.sort(mAdapter.messages, chainComparator);
                     }
                     mAdapter.notifyDataSetChanged();
                 }
             });
         }
 
+        /**
+         * @return The comparator to use to display messages in an ordered
+         *         fashion. Never <code>null</code>.
+         */
+        protected Comparator<MessageInfoHolder> getComparator()
+        {
+            final List<Comparator<MessageInfoHolder>> chain = new ArrayList<Comparator<MessageInfoHolder>>(2 /* we add 2 comparators at most */ );
+
+            {
+                // add the specified comparator
+                final Comparator<MessageInfoHolder> comparator = SORT_COMPARATORS.get(sortType);
+                if (sortAscending)
+                {
+                    chain.add(comparator);
+                }
+                else
+                {
+                    chain.add(new ReverseComparator<MessageInfoHolder>(comparator));
+                }
+            }
+
+            {
+                // add the date comparator if not already specified
+                if (sortType != SORT_TYPE.SORT_DATE)
+                {
+                    final Comparator<MessageInfoHolder> comparator = SORT_COMPARATORS.get(SORT_TYPE.SORT_DATE);
+                    if (sortDateAscending)
+                    {
+                        chain.add(comparator);
+                    }
+                    else
+                    {
+                        chain.add(new ReverseComparator<MessageInfoHolder>(comparator));
+                    }
+                }
+            }
+
+            // build the comparator chain
+            final Comparator<MessageInfoHolder> chainComparator = new ComparatorChain<MessageInfoHolder>(chain);
+
+            return chainComparator;
+        }
+
         public void folderLoading(String folder, boolean loading)
         {
             if (mCurrentFolder != null && mCurrentFolder.name.equals(folder))

src/com/fsck/k9/helper/Utility.java

@@ -11,9 +11,26 @@ import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.UnsupportedEncodingException;
 import java.util.Date;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 public class Utility
 {
+
+    // \u00A0 (non-breaking space) happens to be used by French MUA
+
+    // Note: no longer using the ^ beginning character combined with (...)+
+    // repetition matching as we might want to strip ML tags. Ex:
+    // Re: [foo] Re: RE : [foo] blah blah blah
+    private static final Pattern RESPONSE_PATTERN = Pattern.compile(
+            "((Re|Fw|Fwd|Aw|R\\u00E9f\\.)(\\[\\d+\\])?[\\u00A0 ]?: *)+", Pattern.CASE_INSENSITIVE);
+
+    /**
+     * Mailing-list tag pattern to match strings like "[foobar] "
+     */
+    private static final Pattern TAG_PATTERN = Pattern.compile("\\[[-_a-z0-9]+\\] ",
+            Pattern.CASE_INSENSITIVE);
+
     public final static String readInputStream(InputStream in, String encoding) throws IOException
     {
         InputStreamReader reader = new InputStreamReader(in, encoding);
@@ -384,4 +401,99 @@ public class Utility
 
         return wrappedLine.toString();
     }
+
+    /**
+     * Extract the 'original' subject value, by ignoring leading
+     * response/forward marker and '[XX]' formatted tags (as many mailing-list
+     * softwares do).
+     * 
+     * <p>
+     * Result is also trimmed.
+     * </p>
+     * 
+     * @param subject
+     *            Never <code>null</code>.
+     * @return Never <code>null</code>.
+     */
+    public static String stripSubject(final String subject)
+    {
+        int lastPrefix = 0;
+
+        final Matcher tagMatcher = TAG_PATTERN.matcher(subject);
+        String tag = null;
+        // whether tag stripping logic should be active
+        boolean tagPresent = false;
+        // whether the last action stripped a tag
+        boolean tagStripped = false;
+        if (tagMatcher.find(0))
+        {
+            tagPresent = true;
+            if (tagMatcher.start() == 0)
+            {
+                // found at beginning of subject, considering it an actual tag
+                tag = tagMatcher.group();
+
+                // now need to find response marker after that tag
+                lastPrefix = tagMatcher.end();
+                tagStripped = true;
+            }
+        }
+
+        final Matcher matcher = RESPONSE_PATTERN.matcher(subject);
+
+        // while:
+        // - lastPrefix is within the bounds
+        // - response marker found at lastPrefix position
+        // (to make sure we don't catch response markers that are part of
+        // the actual subject)
+
+        while (lastPrefix < subject.length() - 1
+                && matcher.find(lastPrefix)
+                && matcher.start() == lastPrefix
+                && (!tagPresent || tag == null || subject.regionMatches(matcher.end(), tag, 0,
+                        tag.length())))
+        {
+            lastPrefix = matcher.end();
+
+            if (tagPresent)
+            {
+                tagStripped = false;
+                if (tag == null)
+                {
+                    // attempt to find tag
+                    if (tagMatcher.start() == lastPrefix)
+                    {
+                        tag = tagMatcher.group();
+                        lastPrefix += tag.length();
+                        tagStripped = true;
+                    }
+                }
+                else if (lastPrefix < subject.length() - 1 && subject.startsWith(tag, lastPrefix))
+                {
+                    // Re: [foo] Re: [foo] blah blah blah
+                    //               ^     ^
+                    //               ^     ^
+                    //               ^    new position
+                    //               ^
+                    //              initial position
+                    lastPrefix += tag.length();
+                    tagStripped = true;
+                }
+            }
+        }
+        if (tagStripped)
+        {
+            // restore the last tag
+            lastPrefix -= tag.length();
+        }
+        if (lastPrefix > -1 && lastPrefix < subject.length() - 1)
+        {
+            return subject.substring(lastPrefix).trim();
+        }
+        else
+        {
+            return subject.trim();
+        }
+    }
+
 }

src/com/fsck/k9/provider/MessageProvider.java

@@ -21,8 +21,8 @@ import com.fsck.k9.K9;
 import com.fsck.k9.Preferences;
 import com.fsck.k9.SearchAccount;
 import com.fsck.k9.activity.MessageInfoHolder;
+import com.fsck.k9.activity.MessageList;
 import com.fsck.k9.controller.MessagingController;
-import com.fsck.k9.controller.MessagingController.SORT_TYPE;
 import com.fsck.k9.controller.MessagingListener;
 import com.fsck.k9.mail.Folder;
 import com.fsck.k9.mail.Message;
@@ -107,7 +107,8 @@ public class MessageProvider extends ContentProvider
             int id = -1;
             synchronized (glb_messages)
             {
-                Collections.sort(glb_messages);
+                // TODO dynamically retrieve sort order instead of hardcoding
+                Collections.sort(glb_messages, new MessageList.ReverseComparator<MessageInfoHolder>(new MessageList.DateComparator()));
             }
             MatrixCursor tmpCur = new MatrixCursor(messages_projection);
             synchronized (glb_messages)
@@ -137,12 +138,9 @@ public class MessageProvider extends ContentProvider
 
         public void listLocalMessagesAddMessages(Account account, String folder, List<Message> messages)
         {
-// We will by default sort by DATE desc
-            SORT_TYPE t_sort = SORT_TYPE.SORT_DATE;
-
             for (Message m : messages)
             {
-                MessageInfoHolder m1 = new MessageInfoHolder(context,m,t_sort,false);
+                MessageInfoHolder m1 = new MessageInfoHolder(context, m);
                 glb_messages.add(m1);
             }
         }