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