Bug 262735 - Group by date message up sometimes, e.g. showing yesterday - saturday - yesterday - saturday
Summary: Group by date message up sometimes, e.g. showing yesterday - saturday - yeste...
Status: VERIFIED FIXED
Alias: None
Product: KMail Mobile
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Maemo 5 Linux
: HI normal (vote)
Target Milestone: ---
Assignee: Ludwig Reiter
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-10 09:56 UTC by Bernhard E. Reiter
Modified: 2011-02-16 11:25 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
N900 Screenshot showing messed up group sorting of Kontact Touch Mail (41.70 KB, image/png)
2011-01-10 09:56 UTC, Bernhard E. Reiter
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bernhard E. Reiter 2011-01-10 09:56:27 UTC
Created attachment 55813 [details]
N900 Screenshot showing messed up group sorting of Kontact Touch Mail

Package: kdepim-mobile, N900
Version: 4:4.6~.20110106.1209.gitfde48d5-1maemo1.1211475
Package: libqt4-experimental-gui
Version: 4.7.0~git20101111-0maemo1

Selected my INBOX this morning (Monday) after touchung it the last time
on Friday where I left it with four read emails. After the explicit sync
from the menu and wachting the emails coming in, I had several groups for the same day. See attached screenshot.
(For demonstration purposes I collapsed the groups, this does not change the order.)

INBOX: takes sync options from account
Account: no offline mode; no intervall check

This should stay keyword komo3 at least until we have clarified
that this does not happen on the HTC Touch Pro2.
Comment 1 Bernhard E. Reiter 2011-01-10 11:47:50 UTC
Could also reproduce the issue when searching with N900, got
Dezember 2010, Mittwoch, Dezember 2010
Comment 2 Tobias Koenig 2011-01-10 15:35:14 UTC
Reproducible on Linux/Desktop... will debug it
Comment 3 Tobias Koenig 2011-01-12 13:50:50 UTC
commit b984b66bd8e090d96bcd451a9d19e4af0c5d86ca
branch master
Author: Tobias Koenig <tokoe@kde.org>
Date:   Wed Jan 12 13:51:40 2011 +0100

    Fix grouping of messages in 'most recent' mode
    
    Let ThreadGrouperComparator handle the grouper string, since
    it knows what are the thread leader and the most recent items in
    a thread.
    
    BUG: 262735

diff --git a/mobile/lib/threadgroupermodel.cpp b/mobile/lib/threadgroupermodel.cpp
index 8a00c09..cedb635 100644
--- a/mobile/lib/threadgroupermodel.cpp
+++ b/mobile/lib/threadgroupermodel.cpp
@@ -60,6 +60,11 @@ ThreadGrouperComparator::~ThreadGrouperComparator()
 {
 }
 
+QString ThreadGrouperComparator::grouperString( const Akonadi::Item& ) const
+{
+  return QString();
+}
+
 Akonadi::Item ThreadGrouperComparator::threadItem( const Akonadi::Item &item ) const
 {
   Q_ASSERT( m_grouper );
@@ -186,6 +191,9 @@ QVariant ThreadGrouperModel::data( const QModelIndex &index, int role ) const
 
   if ( role == ThreadIdRole )
     return d->threadRoot( index ).id();
+  else if ( role == GrouperRole ) {
+    return d->m_comparator->grouperString( index.data( Akonadi::EntityTreeModel::ItemRole ).value<Akonadi::Item>() );
+  }
 
   return QSortFilterProxyModel::data( index, role );
 }
@@ -209,6 +217,10 @@ void ThreadGrouperModel::setSourceModel( QAbstractItemModel *sourceModel )
   if ( d->m_dynamicModelRepopulation )
     connect( sourceModel, SIGNAL( dataChanged( const QModelIndex&, const QModelIndex& ) ),
              this, SLOT( populateThreadGrouperModel() ) );
+
+  QHash<int, QByteArray> names = roleNames();
+  names.insert( GrouperRole, "grouperString" );
+  setRoleNames( names );
 }
 
 bool ThreadGrouperModel::lessThan( const QModelIndex &left, const QModelIndex &right ) const
@@ -228,6 +240,7 @@ void ThreadGrouperModel::setThreadingEnabled( bool enabled )
   d->m_threadingEnabled = enabled;
 
   invalidate();
+  reset();
 }
 
 bool ThreadGrouperModel::threadingEnabled() const
diff --git a/mobile/lib/threadgroupermodel.h b/mobile/lib/threadgroupermodel.h
index ba43ea0..0e05171 100644
--- a/mobile/lib/threadgroupermodel.h
+++ b/mobile/lib/threadgroupermodel.h
@@ -62,6 +62,11 @@ class MOBILEUI_EXPORT ThreadGrouperComparator
      */
     virtual bool lessThan( const Akonadi::Item &left, const Akonadi::Item &right ) const = 0;
 
+    /**
+     * Returns the grouper string for the given @p item.
+     */
+    virtual QString grouperString( const Akonadi::Item &item ) const;
+
   protected:
     /**
      * Returns the thread item for @p item.
@@ -102,7 +107,8 @@ class MOBILEUI_EXPORT ThreadGrouperModel : public QSortFilterProxyModel
   public:
     enum CustomRoles {
       // FIXME Fix custom role handling in proxies.
-      ThreadIdRole = Akonadi::EntityTreeModel::UserRole + 30
+      ThreadIdRole = Akonadi::EntityTreeModel::UserRole + 30,
+      GrouperRole
     };
 
     /**
diff --git a/mobile/mail/HeaderView.qml b/mobile/mail/HeaderView.qml
index 7cabc62..3e99446 100644
--- a/mobile/mail/HeaderView.qml
+++ b/mobile/mail/HeaderView.qml
@@ -29,7 +29,6 @@ KPIM.ItemListView {
   property variant checkModel
   property string collapsedSections
   property bool showSections : true
-  property string groupingRole : "dateGroup"
 
   delegate: [
     KPIM.ItemListViewDelegate {
@@ -37,7 +36,7 @@ KPIM.ItemListView {
       showCheckBox : _top.showCheckBox
       checkModel : _top.checkModel
       navigationModel : _top.navigationModel
-      height : (_top.collapsedSections.indexOf(model[groupingRole]) >= 0) ? 0 : (itemListView.height / 7)
+      height : (_top.collapsedSections.indexOf(model.grouperString) >= 0) ? 0 : (itemListView.height / 7)
       clip: true
       summaryContent : [
         QML.Text {
@@ -208,7 +207,7 @@ KPIM.ItemListView {
     }
   ]
 
-  section.property: showSections ? _top.groupingRole : ""
+  section.property: showSections ? "grouperString" : ""
   section.criteria: QML.ViewSection.FullString
   section.delegate: QML.Item {
     id: sectionDelegate
diff --git a/mobile/mail/kmail-mobile.qml b/mobile/mail/kmail-mobile.qml
index 6c77807..66fb2fd 100644
--- a/mobile/mail/kmail-mobile.qml
+++ b/mobile/mail/kmail-mobile.qml
@@ -296,7 +296,6 @@ KPIM.MainView {
         navigationModel : _threadSelector
         showDeleteButton : false // too easy to accidentally hit it, although very useful...
         opacity : threadContentsViewContainer.opacity == 0 ? 1 : 0
-        groupingRole : messageListSettings.groupingRole
         showSections : messageListSettings.groupingRole != ""
       }
       QML.Connections {
diff --git a/mobile/mail/mailthreadgroupercomparator.cpp b/mobile/mail/mailthreadgroupercomparator.cpp
index 35dec0f..c931509 100644
--- a/mobile/mail/mailthreadgroupercomparator.cpp
+++ b/mobile/mail/mailthreadgroupercomparator.cpp
@@ -24,6 +24,10 @@
 #include <akonadi/kmime/messageflags.h>
 #include <messagecore/stringutil.h>
 
+#include <klocale.h>
+#include <kglobal.h>
+#include <kcalendarsystem.h>
+
 MailThreadGrouperComparator::MailThreadGrouperComparator()
   : mSortingOption( SortByDateTimeMostRecent ),
     mIsOutboundCollection( false )
@@ -87,8 +91,8 @@ bool MailThreadGrouperComparator::lessThan( const Akonadi::Item &leftItem, const
         break;
       case SortByDateTimeMostRecent:
         {
-          const KDateTime leftNewest = mostRecentUpdate( leftThreadRootMessage, leftThreadRootItem.id() );
-          const KDateTime rightNewest = mostRecentUpdate( rightThreadRootMessage, rightThreadRootItem.id() );
+          const KDateTime leftNewest = mostRecentDateTimeInThread( leftThreadRootMessage, leftThreadRootItem.id() );
+          const KDateTime rightNewest = mostRecentDateTimeInThread( rightThreadRootMessage, rightThreadRootItem.id() );
 
           if ( leftNewest != rightNewest ) {
             return leftNewest > rightNewest;
@@ -183,6 +187,18 @@ MailThreadGrouperComparator::SortingOption MailThreadGrouperComparator::sortingO
   return mSortingOption;
 }
 
+void MailThreadGrouperComparator::setGroupingOption( GroupingOption option )
+{
+  mGroupingOption = option;
+
+  invalidate();
+}
+
+MailThreadGrouperComparator::GroupingOption MailThreadGrouperComparator::groupingOption() const
+{
+  return mGroupingOption;
+}
+
 void MailThreadGrouperComparator::setIsOutboundCollection( bool outbound )
 {
   mIsOutboundCollection = outbound;
@@ -190,6 +206,72 @@ void MailThreadGrouperComparator::setIsOutboundCollection( bool outbound )
   invalidate();
 }
 
+QString MailThreadGrouperComparator::grouperString( const Akonadi::Item &item ) const
+{
+  KMime::Message::Ptr msg;
+
+  if ( mSortingOption == SortByDateTimeMostRecent ) {
+    const Akonadi::Item::Id newestItem = mostRecentIdInThread( messageForItem( item ), item.id() );
+    msg = messageForItem( Akonadi::Item( newestItem ) );
+  } else {
+    const Akonadi::Item rootItem = threadItem( item );
+    msg = messageForItem( rootItem );
+  }
+
+  if ( mGroupingOption == GroupByDate ) {
+    // simplified version taken from libmessagelist
+    const KDateTime& dt = msg->date()->dateTime();
+    const QDate dDate = dt.date();
+    const KCalendarSystem *calendar = KGlobal::locale()->calendar();
+    int daysAgo = -1;
+    if ( calendar->isValid( dDate ) && calendar->isValid( QDate::currentDate() ) ) {
+      daysAgo = dDate.daysTo( QDate::currentDate() );
+    }
+
+    if ( daysAgo < 0 || !dt.isValid() ) // In the future or invalid
+      return i18n( "Unknown" );
+    else if( daysAgo == 0 ) // Today
+      return i18n( "Today" );
+    else if ( daysAgo == 1 ) // Yesterday
+      return i18n( "Yesterday" );
+    else if ( daysAgo > 1 && daysAgo < calendar->daysInWeek( QDate::currentDate() ) ) // Within last seven days
+      return KGlobal::locale()->calendar()->weekDayName( dDate );
+    else if( calendar->month( dDate ) == calendar->month( QDate::currentDate() ) && calendar->year( dDate ) == calendar->year( QDate::currentDate() ) ) { // within this month
+      const int startOfWeekDaysAgo = ( calendar->daysInWeek( QDate::currentDate() ) + calendar->dayOfWeek( QDate::currentDate() ) -
+                                       KGlobal::locale()->weekStartDay() ) % calendar->daysInWeek( QDate::currentDate() );
+      const int weeksAgo = ( ( daysAgo - startOfWeekDaysAgo ) / calendar->daysInWeek( QDate::currentDate() ) ) + 1;
+      if ( weeksAgo == 0 )
+        return KGlobal::locale()->calendar()->weekDayName( dDate );
+      else
+        return i18np( "One Week Ago", "%1 Weeks Ago", weeksAgo );
+    } else if ( calendar->year( dDate ) == calendar->year( QDate::currentDate() ) ) { // within this year
+      return calendar->monthName( dDate );
+    } else { // in previous years
+      static QHash<int, QString> yearNameHash;
+
+      QString yearName;
+      if ( yearNameHash.contains( dDate.year() ) ) {
+        yearName = yearNameHash.value( dDate.year() );
+      } else {
+        yearName = calendar->yearString( dDate );
+        yearNameHash.insert( dDate.year(), yearName );
+      }
+      return i18nc( "Message Aggregation Group Header: Month name and Year number", "%1 %2", calendar->monthName( dDate ), yearName );
+    }
+  } else if ( mGroupingOption == GroupBySenderReceiver ) {
+    QStringList l;
+    foreach ( const KMime::Types::Mailbox &mbox, msg->from()->mailboxes() ) {
+      if ( mbox.hasName() )
+        l.append( mbox.name() );
+      else
+        l.append( mbox.addrSpec().asPrettyString() );
+    }
+    return l.join( ", " );
+  } else {
+    return QLatin1String( "dummy" );
+  }
+}
+
 void MailThreadGrouperComparator::resetCaches()
 {
   mMessageCache.clear();
@@ -205,18 +287,22 @@ QByteArray MailThreadGrouperComparator::identifierForMessage( const KMime::Messa
   return identifier;
 }
 
-KDateTime MailThreadGrouperComparator::mostRecentUpdate( const KMime::Message::Ptr &threadRoot, Akonadi::Item::Id itemId ) const
+KDateTime MailThreadGrouperComparator::mostRecentDateTimeInThread( const KMime::Message::Ptr &threadRoot, Akonadi::Item::Id itemId ) const
 {
-  const QHash<Akonadi::Item::Id, KDateTime>::const_iterator it = mMostRecentCache.constFind( itemId );
+  const QHash<Akonadi::Item::Id, MostRecentEntry>::const_iterator it = mMostRecentCache.constFind( itemId );
   if ( it != mMostRecentCache.constEnd() )
-    return *it;
+    return (*it).dateTime;
 
   const QSet<QByteArray> messageIds = threadDescendants( identifierForMessage( threadRoot, itemId ) );
 
   KDateTime newest = threadRoot->date()->dateTime();
+  Akonadi::Item::Id newestId = itemId;
 
   if ( messageIds.isEmpty() ) {
-    mMostRecentCache.insert( itemId, newest );
+    MostRecentEntry entry;
+    entry.id = newestId;
+    entry.dateTime = newest;
+    mMostRecentCache.insert( itemId, entry );
     return newest;
   }
 
@@ -227,14 +313,59 @@ KDateTime MailThreadGrouperComparator::mostRecentUpdate( const KMime::Message::P
 
     const KMime::Message::Ptr message = messageForItem( item );
     const KDateTime messageDateTime = message->date()->dateTime();
-    if ( messageDateTime > newest )
+    if ( messageDateTime > newest ) {
       newest = messageDateTime;
+      newestId = item.id();
+    }
   }
 
-  mMostRecentCache.insert( itemId, newest );
+  MostRecentEntry entry;
+  entry.id = newestId;
+  entry.dateTime = newest;
+
+  mMostRecentCache.insert( itemId, entry );
   return newest;
 }
 
+Akonadi::Item::Id MailThreadGrouperComparator::mostRecentIdInThread( const KMime::Message::Ptr &threadRoot, Akonadi::Item::Id itemId ) const
+{
+  const QHash<Akonadi::Item::Id, MostRecentEntry>::const_iterator it = mMostRecentCache.constFind( itemId );
+  if ( it != mMostRecentCache.constEnd() )
+    return (*it).id;
+
+  const QSet<QByteArray> messageIds = threadDescendants( identifierForMessage( threadRoot, itemId ) );
+
+  KDateTime newest = threadRoot->date()->dateTime();
+  Akonadi::Item::Id newestId = itemId;
+
+  if ( messageIds.isEmpty() ) {
+    MostRecentEntry entry;
+    entry.id = newestId;
+    entry.dateTime = newest;
+    mMostRecentCache.insert( itemId, entry );
+    return itemId;
+  }
+
+  foreach ( const QByteArray &messageId, messageIds ) {
+    const Akonadi::Item item = itemForIdentifier( messageId );
+    Q_ASSERT( item.isValid() );
+    Q_ASSERT( item.hasPayload<KMime::Message::Ptr>() );
+
+    const KMime::Message::Ptr message = messageForItem( item );
+    const KDateTime messageDateTime = message->date()->dateTime();
+    if ( messageDateTime > newest )
+      newest = messageDateTime;
+      newestId = item.id();
+  }
+
+  MostRecentEntry entry;
+  entry.id = newestId;
+  entry.dateTime = newest;
+
+  mMostRecentCache.insert( itemId, entry );
+  return itemId;
+}
+
 KMime::Message::Ptr MailThreadGrouperComparator::messageForItem( const Akonadi::Item &item ) const
 {
   const QHash<Akonadi::Item::Id, KMime::Message::Ptr>::const_iterator it = mMessageCache.constFind( item.id() );
diff --git a/mobile/mail/mailthreadgroupercomparator.h b/mobile/mail/mailthreadgroupercomparator.h
index 5c3d696..4fa41e4 100644
--- a/mobile/mail/mailthreadgroupercomparator.h
+++ b/mobile/mail/mailthreadgroupercomparator.h
@@ -39,6 +39,13 @@ class MailThreadGrouperComparator : public ThreadGrouperComparator
       SortByActionItem
     };
 
+    enum GroupingOption
+    {
+      GroupByNone,
+      GroupByDate,
+      GroupBySenderReceiver
+    };
+
     /**
      * Creates a new mail thread grouper comparator.
      */
@@ -67,24 +74,38 @@ class MailThreadGrouperComparator : public ThreadGrouperComparator
     void setSortingOption( SortingOption option );
     SortingOption sortingOption() const;
 
+    void setGroupingOption( GroupingOption option );
+    GroupingOption groupingOption() const;
+
     /**
      * Sets whether the currently compared items come from an outbound mail collection
      * (e.g. outbox, sent or drafts).
      */
     void setIsOutboundCollection( bool outbound );
 
+    virtual QString grouperString( const Akonadi::Item &item ) const;
+
   protected:
     virtual void resetCaches();
 
   private:
     QByteArray identifierForMessage( const KMime::Message::Ptr&, Akonadi::Item::Id ) const;
-    KDateTime mostRecentUpdate( const KMime::Message::Ptr&, Akonadi::Item::Id ) const;
+    KDateTime mostRecentDateTimeInThread( const KMime::Message::Ptr&, Akonadi::Item::Id ) const;
+    Akonadi::Item::Id mostRecentIdInThread( const KMime::Message::Ptr&, Akonadi::Item::Id ) const;
     KMime::Message::Ptr messageForItem( const Akonadi::Item &item ) const;
 
     SortingOption mSortingOption;
+    GroupingOption mGroupingOption;
     bool mIsOutboundCollection;
     mutable QHash<Akonadi::Item::Id, KMime::Message::Ptr> mMessageCache;
-    mutable QHash<Akonadi::Item::Id, KDateTime> mMostRecentCache;
+
+    struct MostRecentEntry
+    {
+      Akonadi::Item::Id id;
+      KDateTime dateTime;
+    };
+
+    mutable QHash<Akonadi::Item::Id, MostRecentEntry> mMostRecentCache;
 };
 
 #endif
diff --git a/mobile/mail/mainview.cpp b/mobile/mail/mainview.cpp
index 35f0d14..cdfae0e 100644
--- a/mobile/mail/mainview.cpp
+++ b/mobile/mail/mainview.cpp
@@ -1753,6 +1753,18 @@ void MainView::messageListSettingsChanged( const MessageListSettings &settings )
       break;
   }
 
+  switch ( settings.groupingOption() ) {
+    case MessageListSettings::GroupByDate:
+      m_grouperComparator->setGroupingOption( MailThreadGrouperComparator::GroupByDate );
+      break;
+    case MessageListSettings::GroupBySenderReceiver:
+      m_grouperComparator->setGroupingOption( MailThreadGrouperComparator::GroupBySenderReceiver );
+      break;
+    case MessageListSettings::GroupByNone:
+      m_grouperComparator->setGroupingOption( MailThreadGrouperComparator::GroupByNone );
+      break;
+  }
+
   m_threadGrouperModel->setThreadingEnabled( settings.useThreading() );
 
   m_threadGrouperModel->sort( 0, settings.sortingOrder() );
Comment 4 Bernhard E. Reiter 2011-01-18 17:25:34 UTC
It does not seem to be fixed completely with 
windows-ce/Kontact-Mobile/testing/2011-01-17-02-23/
I still get groups like:

"Mittwoch"
"Vor 2 Wochen"
"Dezember 2010"
Vor 2 Wochen"
"Dezember 2010"
Comment 5 Bernhard E. Reiter 2011-01-18 17:26:30 UTC
Reproduced with a freshly configured account, just selected INBOX and stayed in there until it was synced.
Comment 6 Bernhard E. Reiter 2011-01-18 17:31:18 UTC
Maillist view settings:
Sorting: "nach neuster nachricht in der diskussion" "aufsteigend"
   [ ] not checked "Ordner verwendet immer diese Sortierung"
Grouping: "nach neuster nachricht in der diskussion"
   [X] checked "Diskussionen anzeigen"
Comment 7 Bernhard E. Reiter 2011-01-19 10:14:33 UTC
Second time I've started the account, the grouping order was fine,
so I _guess_ it only happens when (several) emails come in. A situation
which can happen during any sync.
Comment 8 Tobias Koenig 2011-02-02 17:52:39 UTC
Git commit f947dd12a27fe1a79a20416a085592073988bbd0 by Tobias Koenig.
Committed on 02/02/11 at 17:54.
Pushed by tokoe into branch 'komo3'.

Fix sorting/grouping of incoming messages

We have to resort the message list whenever new messages
are inserted to make it work in 'most-recent-message-in-thread'
mode.

BUG: 262735

M  +9    -0    mobile/lib/threadgroupermodel.cpp     
M  +1    -0    mobile/lib/threadgroupermodel.h     
M  +2    -1    mobile/mail/mailthreadgroupercomparator.cpp     

http://commits.kde.org/kdepim/f947dd12a27fe1a79a20416a085592073988bbd0
Comment 9 Tobias Koenig 2011-02-02 18:06:50 UTC
Git commit 5a808eb3b87641f372f0ee4ebb336e38d3fb8521 by Tobias Koenig.
Committed on 02/02/11 at 17:54.
Pushed by tokoe into branch 'master'.

Fix sorting/grouping of incoming messages

We have to resort the message list whenever new messages
are inserted to make it work in 'most-recent-message-in-thread'
mode.

BUG: 262735

M  +9    -0    mobile/lib/threadgroupermodel.cpp     
M  +1    -0    mobile/lib/threadgroupermodel.h     
M  +2    -1    mobile/mail/mailthreadgroupercomparator.cpp     

http://commits.kde.org/kdepim/5a808eb3b87641f372f0ee4ebb336e38d3fb8521
Comment 10 Bjoern Ricks 2011-02-16 11:25:00 UTC
Seems to be fixed in 4:4.6~.20110214.1233.gitb901f2e-1maemo1.1219908 on maemo