Quick fix for not working history in demobrowser

Current implementation of history manager seems to
be not straight forward. To display history we use QList to keep
history items plus model adapter in form of HistoryModel
(QAbstractTableModel) plus HistoryFilterModel to avoid
duplication, plus HistoryTreeModel to make a tree view with
split for dates, plus TreeProxyModel on top to enable
sorting. This approach unfortunately falls apart when items
should be deleted from the model by treeView. This ends
badly with corrupted cached values.

This fix removes history items using history manager.
It also abandons HistoryTreeModel since frequent sourceReset calls make
it unusable. If split for dates it desired the better approach
would be to implement history tree model already at HistoryManager
level to avoid unmaintainable code.

Task-number: QTBUG-49913
Task-number: QTBUG-50255
Change-Id: Ic9cebb52b623bd453119e11b5e907eaa90609e34
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

1 files changed, 57 insertions, 42 deletions

diff --git a/examples/webenginewidgets/demobrowser/history.cpp b/examples/webenginewidgets/demobrowser/history.cpp
index 615582fe2..72f96ed4e 100644
--- a/examples/webenginewidgets/demobrowser/history.cpp
+++ b/examples/webenginewidgets/demobrowser/history.cpp
@@ -91,7 +91,7 @@ HistoryManager::~HistoryManager()
     m_saveTimer->saveIfNeccessary();
 }
 
-QList<HistoryItem> HistoryManager::history() const
+QList<HistoryItem> &HistoryManager::history()
 {
     return m_history;
 }
@@ -110,6 +110,15 @@ void HistoryManager::addHistoryEntry(const QString &url)
     addHistoryItem(item);
 }
 
+void HistoryManager::removeHistoryEntry(const QString &url)
+{
+    QUrl cleanUrl(url);
+    cleanUrl.setPassword(QString());
+    cleanUrl.setHost(cleanUrl.host().toLower());
+    HistoryItem item(cleanUrl.toString(), QDateTime::currentDateTime());
+    removeHistoryItem(item);
+}
+
 void HistoryManager::setHistory(const QList<HistoryItem> &history, bool loadedAndSorted)
 {
     m_history = history;
@@ -163,7 +172,7 @@ void HistoryManager::checkForExpired()
         }
         if (nextTimeout > 0)
             break;
-        HistoryItem item = m_history.takeLast();
+        const HistoryItem& item = m_history.last();
         // remove from saved file also
         m_lastSavedUrl = QString();
         emit entryRemoved(item);
@@ -178,12 +187,21 @@ void HistoryManager::addHistoryItem(const HistoryItem &item)
     if (BrowserApplication::instance()->privateBrowsing())
         return;
 
-    m_history.prepend(item);
     emit entryAdded(item);
     if (m_history.count() == 1)
         checkForExpired();
 }
 
+void HistoryManager::removeHistoryItem(const HistoryItem &item)
+{
+    for (int i = m_history.count() - 1 ; i >= 0; --i) {
+        if (item.url == m_history.at(i).url) {
+            //delete all related entries with that url
+            emit entryRemoved(m_history.at(i));
+        }
+    }
+}
+
 void HistoryManager::updateHistoryItem(const QUrl &url, const QString &title)
 {
     for (int i = 0; i < m_history.count(); ++i) {
@@ -214,7 +232,6 @@ void HistoryManager::setHistoryLimit(int limit)
 
 void HistoryManager::clear()
 {
-    m_history.clear();
     m_lastSavedUrl = QString();
     emit historyReset();
     m_saveTimer->changeOccurred();
@@ -364,10 +381,10 @@ HistoryModel::HistoryModel(HistoryManager *history, QObject *parent)
     connect(m_history, SIGNAL(historyReset()),
             this, SLOT(historyReset()));
     connect(m_history, SIGNAL(entryRemoved(HistoryItem)),
-            this, SLOT(historyReset()));
+            this, SLOT(entryRemoved(HistoryItem)));
 
     connect(m_history, SIGNAL(entryAdded(HistoryItem)),
-            this, SLOT(entryAdded()));
+            this, SLOT(entryAdded(HistoryItem)));
     connect(m_history, SIGNAL(entryUpdated(int)),
             this, SLOT(entryUpdated(int)));
 }
@@ -375,15 +392,26 @@ HistoryModel::HistoryModel(HistoryManager *history, QObject *parent)
 void HistoryModel::historyReset()
 {
     beginResetModel();
+    m_history->history().clear();
     endResetModel();
 }
 
-void HistoryModel::entryAdded()
+void HistoryModel::entryAdded(const HistoryItem &item)
 {
     beginInsertRows(QModelIndex(), 0, 0);
+    m_history->history().prepend(item);
     endInsertRows();
 }
 
+void HistoryModel::entryRemoved(const HistoryItem &item)
+{
+    int index = m_history->history().indexOf(item);
+    Q_ASSERT(index > -1);
+    beginRemoveRows(QModelIndex(),index, index);
+    m_history->history().takeAt(index);
+    endRemoveRows();
+}
+
 void HistoryModel::entryUpdated(int offset)
 {
     QModelIndex idx = index(offset, 0);
@@ -458,12 +486,9 @@ bool HistoryModel::removeRows(int row, int count, const QModelIndex &parent)
         return false;
     int lastRow = row + count - 1;
     beginRemoveRows(parent, row, lastRow);
-    QList<HistoryItem> lst = m_history->history();
+    QList<HistoryItem> &lst = m_history->history();
     for (int i = lastRow; i >= row; --i)
         lst.removeAt(i);
-    disconnect(m_history, SIGNAL(historyReset()), this, SLOT(historyReset()));
-    m_history->setHistory(lst);
-    connect(m_history, SIGNAL(historyReset()), this, SLOT(historyReset()));
     endRemoveRows();
     return true;
 }
@@ -654,8 +679,6 @@ TreeProxyModel::TreeProxyModel(QObject *parent) : QSortFilterProxyModel(parent)
 
 bool TreeProxyModel::filterAcceptsRow(int source_row, const QModelIndex &source_parent) const
 {
-    if (!source_parent.isValid())
-        return true;
     return QSortFilterProxyModel::filterAcceptsRow(source_row, source_parent);
 }
 
@@ -668,14 +691,16 @@ HistoryDialog::HistoryDialog(QWidget *parent, HistoryManager *setHistory) : QDia
     tree->setUniformRowHeights(true);
     tree->setSelectionBehavior(QAbstractItemView::SelectRows);
     tree->setTextElideMode(Qt::ElideMiddle);
-    QAbstractItemModel *model = history->historyTreeModel();
+    QAbstractItemModel *model = history->historyFilterModel();
     TreeProxyModel *proxyModel = new TreeProxyModel(this);
     connect(search, SIGNAL(textChanged(QString)),
             proxyModel, SLOT(setFilterFixedString(QString)));
     connect(removeButton, SIGNAL(clicked()), tree, SLOT(removeOne()));
-    connect(removeAllButton, SIGNAL(clicked()), history, SLOT(clear()));
+    connect(removeAllButton, SIGNAL(clicked()), tree, SLOT(removeAll()));
     proxyModel->setSourceModel(model);
+    proxyModel->setFilterKeyColumn(1);
     tree->setModel(proxyModel);
+    tree->setSortingEnabled(true);
     tree->setExpanded(proxyModel->index(0, 0), true);
     tree->setAlternatingRowColors(true);
     QFontMetrics fm(font());
@@ -729,25 +754,13 @@ HistoryFilterModel::HistoryFilterModel(QAbstractItemModel *sourceModel, QObject
     setSourceModel(sourceModel);
 }
 
-int HistoryFilterModel::historyLocation(const QString &url) const
-{
-    load();
-    if (!m_historyHash.contains(url))
-        return 0;
-    return sourceModel()->rowCount() - m_historyHash.value(url);
-}
-
-QVariant HistoryFilterModel::data(const QModelIndex &index, int role) const
-{
-    return QAbstractProxyModel::data(index, role);
-}
-
 void HistoryFilterModel::setSourceModel(QAbstractItemModel *newSourceModel)
 {
+    beginResetModel();
     if (sourceModel()) {
         disconnect(sourceModel(), SIGNAL(modelReset()), this, SLOT(sourceReset()));
         disconnect(sourceModel(), SIGNAL(dataChanged(QModelIndex,QModelIndex)),
-                   this, SLOT(dataChanged(QModelIndex,QModelIndex)));
+                   this, SLOT(sourceDataChanged(QModelIndex,QModelIndex)));
         disconnect(sourceModel(), SIGNAL(rowsInserted(QModelIndex,int,int)),
                 this, SLOT(sourceRowsInserted(QModelIndex,int,int)));
         disconnect(sourceModel(), SIGNAL(rowsRemoved(QModelIndex,int,int)),
@@ -757,7 +770,6 @@ void HistoryFilterModel::setSourceModel(QAbstractItemModel *newSourceModel)
     QAbstractProxyModel::setSourceModel(newSourceModel);
 
     if (sourceModel()) {
-        m_loaded = false;
         connect(sourceModel(), SIGNAL(modelReset()), this, SLOT(sourceReset()));
         connect(sourceModel(), SIGNAL(dataChanged(QModelIndex,QModelIndex)),
                    this, SLOT(sourceDataChanged(QModelIndex,QModelIndex)));
@@ -766,6 +778,8 @@ void HistoryFilterModel::setSourceModel(QAbstractItemModel *newSourceModel)
         connect(sourceModel(), SIGNAL(rowsRemoved(QModelIndex,int,int)),
                 this, SLOT(sourceRowsRemoved(QModelIndex,int,int)));
     }
+    load();
+    endResetModel();
 }
 
 void HistoryFilterModel::sourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight)
@@ -780,14 +794,13 @@ QVariant HistoryFilterModel::headerData(int section, Qt::Orientation orientation
 
 void HistoryFilterModel::sourceReset()
 {
-    m_loaded = false;
     beginResetModel();
+    load();
     endResetModel();
 }
 
 int HistoryFilterModel::rowCount(const QModelIndex &parent) const
 {
-    load();
     if (parent.isValid())
         return 0;
     return m_historyHash.count();
@@ -800,14 +813,12 @@ int HistoryFilterModel::columnCount(const QModelIndex &parent) const
 
 QModelIndex HistoryFilterModel::mapToSource(const QModelIndex &proxyIndex) const
 {
-    load();
     int sourceRow = sourceModel()->rowCount() - proxyIndex.internalId();
     return sourceModel()->index(sourceRow, proxyIndex.column());
 }
 
 QModelIndex HistoryFilterModel::mapFromSource(const QModelIndex &sourceIndex) const
 {
-    load();
     QString url = sourceIndex.data(HistoryModel::UrlStringRole).toString();
     if (!m_historyHash.contains(url))
         return QModelIndex();
@@ -833,7 +844,6 @@ QModelIndex HistoryFilterModel::mapFromSource(const QModelIndex &sourceIndex) co
 
 QModelIndex HistoryFilterModel::index(int row, int column, const QModelIndex &parent) const
 {
-    load();
     if (row < 0 || row >= rowCount(parent)
         || column < 0 || column >= columnCount(parent))
         return QModelIndex();
@@ -848,8 +858,6 @@ QModelIndex HistoryFilterModel::parent(const QModelIndex &) const
 
 void HistoryFilterModel::load() const
 {
-    if (m_loaded)
-        return;
     m_sourceRow.clear();
     m_historyHash.clear();
     m_historyHash.reserve(sourceModel()->rowCount());
@@ -861,15 +869,12 @@ void HistoryFilterModel::load() const
             m_historyHash[url] = sourceModel()->rowCount() - i;
         }
     }
-    m_loaded = true;
 }
 
 void HistoryFilterModel::sourceRowsInserted(const QModelIndex &parent, int start, int end)
 {
     Q_ASSERT(start == end && start == 0);
     Q_UNUSED(end);
-    if (!m_loaded)
-        return;
     QModelIndex idx = sourceModel()->index(start, 0, parent);
     QString url = idx.data(HistoryModel::UrlStringRole).toString();
     if (m_historyHash.contains(url)) {
@@ -1174,6 +1179,7 @@ bool HistoryTreeModel::removeRows(int row, int count, const QModelIndex &parent)
 
 void HistoryTreeModel::setSourceModel(QAbstractItemModel *newSourceModel)
 {
+    beginResetModel();
     if (sourceModel()) {
         disconnect(sourceModel(), SIGNAL(modelReset()), this, SLOT(sourceReset()));
         disconnect(sourceModel(), SIGNAL(layoutChanged()), this, SLOT(sourceReset()));
@@ -1181,6 +1187,8 @@ void HistoryTreeModel::setSourceModel(QAbstractItemModel *newSourceModel)
                 this, SLOT(sourceRowsInserted(QModelIndex,int,int)));
         disconnect(sourceModel(), SIGNAL(rowsRemoved(QModelIndex,int,int)),
                 this, SLOT(sourceRowsRemoved(QModelIndex,int,int)));
+        disconnect(sourceModel(), &QAbstractItemModel::dataChanged, this,
+                   &HistoryTreeModel::sourceDataChanged);
     }
 
     QAbstractProxyModel::setSourceModel(newSourceModel);
@@ -1192,9 +1200,9 @@ void HistoryTreeModel::setSourceModel(QAbstractItemModel *newSourceModel)
                 this, SLOT(sourceRowsInserted(QModelIndex,int,int)));
         connect(sourceModel(), SIGNAL(rowsRemoved(QModelIndex,int,int)),
                 this, SLOT(sourceRowsRemoved(QModelIndex,int,int)));
+        connect(sourceModel(), &QAbstractItemModel::dataChanged, this,
+                &HistoryTreeModel::sourceDataChanged);
     }
-
-    beginResetModel();
     endResetModel();
 }
 
@@ -1283,3 +1291,10 @@ void HistoryTreeModel::sourceRowsRemoved(const QModelIndex &parent, int start, i
         endRemoveRows();
     }
 }
+
+void HistoryTreeModel::sourceDataChanged(const QModelIndex &topLeft,
+                                         const QModelIndex &bottomRight,
+                                         const QVector<int> roles)
+{
+     emit dataChanged(mapFromSource(topLeft), mapFromSource(bottomRight), roles);
+}