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>

4 files changed, 72 insertions, 50 deletions

diff --git a/examples/webenginewidgets/demobrowser/edittreeview.cpp b/examples/webenginewidgets/demobrowser/edittreeview.cpp
index 872191015..001491032 100644
--- a/examples/webenginewidgets/demobrowser/edittreeview.cpp
+++ b/examples/webenginewidgets/demobrowser/edittreeview.cpp
@@ -39,6 +39,8 @@
 ****************************************************************************/
 
 #include "edittreeview.h"
+#include "browserapplication.h"
+#include "history.h"
 
 #include <QtGui/QKeyEvent>
 
@@ -63,13 +65,12 @@ void EditTreeView::removeOne()
     if (!model())
         return;
     QModelIndex ci = currentIndex();
-    int row = ci.row();
-    model()->removeRow(row, ci.parent());
+    BrowserApplication::historyManager()->removeHistoryEntry(model()->data(ci,HistoryModel::UrlStringRole).toString());
 }
 
 void EditTreeView::removeAll()
 {
     if (!model())
         return;
-    model()->removeRows(0, model()->rowCount(rootIndex()), rootIndex());
+    BrowserApplication::historyManager()->clear();
 }
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);
+}
diff --git a/examples/webenginewidgets/demobrowser/history.h b/examples/webenginewidgets/demobrowser/history.h
index d8d9e6abe..2766dd91e 100644
--- a/examples/webenginewidgets/demobrowser/history.h
+++ b/examples/webenginewidgets/demobrowser/history.h
@@ -93,14 +93,16 @@ public:
     ~HistoryManager();
 
     bool historyContains(const QString &url) const;
+
     void addHistoryEntry(const QString &url);
+    void removeHistoryEntry(const QString &url);
 
     void updateHistoryItem(const QUrl &url, const QString &title);
 
     int historyLimit() const;
     void setHistoryLimit(int limit);
 
-    QList<HistoryItem> history() const;
+    QList<HistoryItem>& history();
     void setHistory(const QList<HistoryItem> &history, bool loadedAndSorted = false);
 
     // History manager keeps around these models for use by the completer and other classes
@@ -118,6 +120,7 @@ private slots:
 
 protected:
     void addHistoryItem(const HistoryItem &item);
+    void removeHistoryItem(const HistoryItem &item);
 
 private:
     void load();
@@ -139,7 +142,8 @@ class HistoryModel : public QAbstractTableModel
 
 public slots:
     void historyReset();
-    void entryAdded();
+    void entryAdded(const HistoryItem &item);
+    void entryRemoved(const HistoryItem &item);
     void entryUpdated(int offset);
 
 public:
@@ -175,7 +179,6 @@ public:
 
     inline bool historyContains(const QString &url) const
         { load(); return m_historyHash.contains(url); }
-    int historyLocation(const QString &url) const;
 
     QModelIndex mapFromSource(const QModelIndex &sourceIndex) const;
     QModelIndex mapToSource(const QModelIndex &proxyIndex) const;
@@ -186,7 +189,6 @@ public:
     QModelIndex index(int, int, const QModelIndex& = QModelIndex()) const;
     QModelIndex parent(const QModelIndex& index= QModelIndex()) const;
     bool removeRows(int row, int count, const QModelIndex &parent = QModelIndex());
-    QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const;
 
 private slots:
     void sourceReset();
@@ -304,7 +306,8 @@ private slots:
     void sourceReset();
     void sourceRowsInserted(const QModelIndex &parent, int start, int end);
     void sourceRowsRemoved(const QModelIndex &parent, int start, int end);
-
+    void sourceDataChanged(const QModelIndex &topLeft, const QModelIndex &bottomRight,
+                           const QVector<int> roles);
 private:
     int sourceDateRow(int row) const;
     mutable QList<int> m_sourceRowCache;
diff --git a/examples/webenginewidgets/demobrowser/tabwidget.cpp b/examples/webenginewidgets/demobrowser/tabwidget.cpp
index 3f859c386..fee34f6d9 100644
--- a/examples/webenginewidgets/demobrowser/tabwidget.cpp
+++ b/examples/webenginewidgets/demobrowser/tabwidget.cpp
@@ -714,6 +714,9 @@ void TabWidget::webViewUrlChanged(const QUrl &url)
     int index = webViewIndex(webView);
     if (-1 != index) {
         m_tabBar->setTabData(index, url);
+        HistoryManager *manager = BrowserApplication::historyManager();
+        if (url.isValid())
+            manager->addHistoryEntry(url.toString());
     }
     emit tabsChanged();
 }