On Mon, 18 Apr 2011 18:35:27 +0300
ext-arun.1.ravind...@nokia.com wrote:

> From: Arun Ravindran <ext-arun.1.ravind...@nokia.com>
> 
> This patch fixes the following issue:
> 
> 1) A fresh device, with no call history.
> 2) Start dialer and receive and incoming call
> 3) Complete the call by disconnecting it.
> 4) See the call history (everything is fine)
> 5) close dialer and restart it again.
> 6) See call history, Shows the unix epoch or a time close to that.

Generally, the patch is ok, but I have a few concerns in appendRows and
getHistoryFinished, as noted below.

Also one variable spelling error.

Shane...

> 
> This is because of wrong usage of QSettings.
> 
> When the dialer restarted the values of QSetting
> @ /home/meego/.config/com.meego.conf is
> 
> [CallHistory]
> CallHistory\1302858505\LineID=+358403445507
> CallHistory\1302858505\Type=1
> CallHistory\1302858505\Start=1302858505
> CallHistory\1302858505\End=1302858516
> 
> The code in HistoryTableModel::insertRows() will fail to read such a
> call history.
> 
> 1) beginGroup() @ [CallHistory]
> 2) childGroups() @ CallHistory.
> 3) max = events.size(); is 1 so gets in to the loop.
> 4) all values returned are 0.
> 
> The appendRows called from the getHistoryFinished also needed change
> as it works on the sub group and not the main group.
> ---
>  src/historytablemodel.cpp |   68
> +++++++++++++++++++++++++++-----------------
> src/managerproxy.cpp      |    8 +++++ 2 files changed, 50
> insertions(+), 26 deletions(-)
> 
> diff --git a/src/historytablemodel.cpp b/src/historytablemodel.cpp
> index 25bb122..efd7140 100644
> --- a/src/historytablemodel.cpp
> +++ b/src/historytablemodel.cpp
> @@ -113,43 +113,50 @@ bool HistoryTableModel::insertRows(int row, int
> count, QSettings *cache = historyProxy->cache();
>  
>      cache->beginGroup("CallHistory");
> -    QStringList events = cache->childGroups();
> +    QStringList groups = cache->childGroups();
>  
> -    if (events.size() == 0) {
> +    if (groups.size() == 0) {
>          qWarning() << QString("[HistoryTableModel] Empty call
> history log!"); return true;
>      }
> +    foreach (QString group, groups) {
>  
> -    // Special case to just load all data from cache
> -    if (max < 0)
> -        max = events.size();
> +        cache->beginGroup(group);
>  
> -    beginInsertRows(parent, row, row + max - 1);
> -    foreach (QString key, events) {
> +        QStringList events = cache->childGroups();
>  
> -        // Stop before end of cache if we hit count
> -        if (i > max) break;
> +        // Special case to just load all data from cache
> +        if (max < 0)
> +            max = events.size();
>  
> -        cache->beginGroup(key);
> +        beginInsertRows(parent, row, row + max - 1);
> +        foreach (QString key, events) {
>  
> -        uint start = cache->value("Start").toUInt(); // Call start
> time
> -        uint end   = cache->value("End").toUInt();   // Call end time
> +            // Stop before end of cache if we hit count
> +            if (i > max) break;
>  
> -        // add the column data to a new row
> -        newRow.clear();
> -        newRow << cache->value("LineID").toString(); // Phone Number
> -        newRow << cache->value("Type").toString();   // Call
> direction type
> -        newRow << toOfonoString(start);              // Call start
> time
> -        newRow << toOfonoString(end);                // Call end time
> +            cache->beginGroup(key);
> +
> +            uint start = cache->value("Start").toUInt(); // Call
> start time
> +            uint end   = cache->value("End").toUInt();   // Call end
> time +
> +            // add the column data to a new row
> +            newRow.clear();
> +            newRow << cache->value("LineID").toString(); // Phone
> Number
> +            newRow << cache->value("Type").toString();   // Call
> direction type
> +            newRow << toOfonoString(start);              // Call
> start time
> +            newRow << toOfonoString(end);                // Call end
> time 
>  #ifdef WANT_DEBUG
> -        qDebug() << QString("[HistoryTableModel] Appending row: %1")
> +            qDebug() << QString("[HistoryTableModel] Appending row:
> %1") .arg(newRow.join("\t"));
>  #endif
> -        // add the row data to the vector
> -        m_data.append(newRow);
> -        cache->endGroup();
> -        i++;
> +            // add the row data to the vector
> +            m_data.append(newRow);
> +            cache->endGroup();
> +            i++;
> +        }
> +     cache->endGroup();
>      }
>      cache->endGroup();
>      endInsertRows();
> @@ -159,7 +166,7 @@ bool HistoryTableModel::insertRows(int row, int
> count, void HistoryTableModel::appendRows(QStringList keys)
>  {
>      TRACE
> -
> +    bool subgroup = false;   
>      int max = 0;
>      int i = 0;
>      QStringList newRow = QStringList();
> @@ -167,12 +174,19 @@ void HistoryTableModel::appendRows(QStringList
> keys) QSettings *cache = mp->history()->cache();
>  
>      cache->beginGroup("CallHistory");
> -    QStringList events = cache->childGroups();
> +    QStringList grpups = cache->childGroups();

I think you mean "groups" here, not grpups ;)

>  
> -    if (events.size() == 0) {
> +    if (groups.size() == 0) {
>          qWarning() << QString("[HistoryTableModel] Empty call
> history log!"); return;
>      }
> +    //Keys contains the actual data and not the subgroup CallHistory.
> +    // Since there is only one subgroup,we can access it directly.
> +    // and index at the sub group
> +    if(groups.size() && groups.at(0) != keys.at(0)) {

This assumes that com.meego.conf will only ever contain CallHistory and
that it will be the first entry, correct?  Wouldn't it be better to use
something like "groups.contains(keys.at(0))" instead?

> +        cache->beginGroup(groups.at(0));

Similarly, wouldn't we want to use "groups.indexOf(keys.at(0))" rather
than "0" here, just in case other apps write to this QSettings file?

> +        subgroup=true;
> +    }
>  
>      // Special case to just load all data from cache
>      max = keys.size();
> @@ -203,6 +217,8 @@ void HistoryTableModel::appendRows(QStringList
> keys) cache->endGroup();
>          i++;
>      }
> +    if(subgroup) 
> +        cache->endGroup();
>      cache->endGroup();
>      endInsertRows();
>      return;
> diff --git a/src/managerproxy.cpp b/src/managerproxy.cpp
> index 0023a0d..efa8099 100644
> --- a/src/managerproxy.cpp
> +++ b/src/managerproxy.cpp
> @@ -301,6 +301,11 @@ void
> HistoryProxy::getHistoryFinished(QDBusPendingCallWatcher *call)
> QList<CallHistoryEvent> missedCalls; 
>              m_cache->beginGroup("CallHistory");
> +         QStringList groups = m_cache->childGroups();
> +
> +            // Assuming the CallHistory as the only subgroup
> +         if(groups.size())

Lets try not to make this assumption, as noted above.  We never know if
another app will write to this QSettings file.

> +                m_cache->beginGroup(groups.at(0));
>  
>              // Cache the new events
>              QStringList ids;
> @@ -324,6 +329,9 @@ void
> HistoryProxy::getHistoryFinished(QDBusPendingCallWatcher *call)
> missedCalls << e; }
>              }
> +            if(groups.size())
> +                m_cache->endGroup();
> +
>              m_cache->endGroup(); // "CallHistory"
>  
>              if (!missedCalls.isEmpty())
_______________________________________________
MeeGo-handset mailing list
MeeGo-handset@lists.meego.com
http://lists.meego.com/listinfo/meego-handset

Reply via email to