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
