Merge pull request #118 from christianhelle/feature/query-sorting-freeze-fixes-4530f6

Open query results in their own order instead of sorting them backwards

Author: christianhelle

src/gui/queryresultpresenter.cpp

@@ -5,6 +5,26 @@
 #include <QItemSelectionModel>
 #include <QPointer>
 #include <QAbstractItemModel>
+#include <QHeaderView>
+
+namespace {
+    // Qt starts a header off pointing at the first column in descending order,
+    // and enabling sorting on a view sorts whatever model is bound to it there
+    // and then. A paged result answers a sort by re-running its statement
+    // wrapped in an ORDER BY, so every result used to be executed twice and
+    // shown backwards before anyone had asked for an order -- and the wrap
+    // undoes the paging, because the database has to order the whole result set
+    // before it can hand back the first page.
+    //
+    // Clearing the indicator first leaves the rows in the order the statement
+    // produced them and makes a sort request for column -1, which the paged
+    // model ignores. The first click on any column then sorts ascending, which
+    // is Qt's default order for a section the indicator is not already on.
+    void enableSortingAscending(QTableView *view) {
+        view->horizontalHeader()->setSortIndicator(-1, Qt::AscendingOrder);
+        view->setSortingEnabled(true);
+    }
+}
 
 QueryResultPresenter::QueryResultPresenter(QWidget *parent)
     : widget(parent) {
@@ -49,8 +69,10 @@ void QueryResultPresenter::present(const QList<QAbstractItemModel *> &models) {
 
         // Parent the model to its view so it is destroyed along with the view.
         model->setParent(tablePtr);
+        // Sorting is enabled before the model is bound, so binding it does not
+        // sort it.
+        enableSortingAscending(tablePtr);
         tablePtr->setModel(model);
-        tablePtr->setSortingEnabled(true);
         tablePtr->setGeometry(QRect(0, yOffset, width, height));
         tablePtr->show();
         this->tableViews.append(tablePtr);
@@ -73,8 +95,10 @@ void QueryResultPresenter::presentToView(QTableView *view, QAbstractItemModel *m
     const QPointer<QItemSelectionModel> previousSelection = view->selectionModel();
 
     model->setParent(view);
+    // As in present(): sorting is set up while the outgoing model is still
+    // bound, because doing it afterwards would sort the table on the way in.
+    enableSortingAscending(view);
     view->setModel(model);
     delete previousSelection.data();
     delete previousModel.data();
-    view->setSortingEnabled(true);
 }

tests/test_queryresultpresenter.cpp

@@ -1,11 +1,74 @@
 #include <gtest/gtest.h>
 #include <QAbstractItemModel>
+#include <QApplication>
+#include <QHeaderView>
+#include <QMouseEvent>
 #include <QStandardItemModel>
 #include <QTableView>
 #include <QWidget>
+#include <algorithm>
+#include <memory>
 
 #include "gui/queryresultpresenter.h"
 
+namespace {
+    // What a view asked a model to sort by. A paged result answers a sort by
+    // re-running its statement, so an unasked-for sort is both the wrong order
+    // and a second execution of the query.
+    //
+    // The record is kept outside the model and shared with it, because binding
+    // a model to a view deletes the one it replaces: a test that reads this
+    // after the next result arrives would otherwise be reading freed memory.
+    struct SortLog {
+        QList<int> columns{};
+        QList<Qt::SortOrder> orders{};
+
+        // Sorting by column -1 is Qt's "leave it in its natural order", so it
+        // does not count as having sorted anything.
+        [[nodiscard]] bool sorted() const {
+            return std::any_of(columns.begin(),
+                               columns.end(),
+                               [](const int column) { return column >= 0; });
+        }
+    };
+
+    class SortSpyModel final : public QStandardItemModel {
+    public:
+        SortSpyModel(const int rows, const int columns, std::shared_ptr<SortLog> log)
+            : QStandardItemModel(rows, columns), log(std::move(log)) {
+        }
+
+        void sort(const int column, const Qt::SortOrder order) override {
+            if (this->log != nullptr) {
+                this->log->columns.append(column);
+                this->log->orders.append(order);
+            }
+            // Column -1 is Qt asking for no particular order, which is not
+            // something to hand to a model that orders rows for real.
+            if (column >= 0)
+                QStandardItemModel::sort(column, order);
+        }
+
+    private:
+        std::shared_ptr<SortLog> log;
+    };
+
+    void clickHeaderSection(const QTableView *view, const int section) {
+        const auto *header = view->horizontalHeader();
+        const QPoint position(
+            header->sectionViewportPosition(section) + header->sectionSize(section) / 2,
+            header->viewport()->height() / 2);
+        const QPoint global = header->viewport()->mapToGlobal(position);
+        QMouseEvent press(QEvent::MouseButtonPress, position, global,
+                          Qt::LeftButton, Qt::LeftButton, Qt::NoModifier);
+        // A release reports the buttons still held afterwards, which is none.
+        QMouseEvent release(QEvent::MouseButtonRelease, position, global,
+                            Qt::LeftButton, Qt::NoButton, Qt::NoModifier);
+        QApplication::sendEvent(header->viewport(), &press);
+        QApplication::sendEvent(header->viewport(), &release);
+    }
+}
+
 class QueryResultPresenterTest : public ::testing::Test {
 protected:
     void SetUp() override {
@@ -16,8 +79,9 @@ class QueryResultPresenterTest : public ::testing::Test {
     }
 
     // Stands in for a PagedResult; the presenter only ever binds models.
-    static QAbstractItemModel *modelWithRows(const int rows) {
-        auto *model = new QStandardItemModel(rows, 2);
+    static SortSpyModel *modelWithRows(const int rows,
+                                       std::shared_ptr<SortLog> log = nullptr) {
+        auto *model = new SortSpyModel(rows, 2, std::move(log));
         for (int row = 0; row < rows; ++row) {
             model->setItem(row, 0, new QStandardItem(QString::number(row)));
             model->setItem(row, 1, new QStandardItem("name" + QString::number(row)));
@@ -97,3 +161,60 @@ TEST_F(QueryResultPresenterTest, PresentToViewIgnoresMissingModel) {
     ASSERT_NE(view.model(), nullptr) << "a failed preview cleared the view";
     EXPECT_EQ(view.model()->rowCount(), 3);
 }
+
+// Qt's header starts out on the first column in descending order, and enabling
+// sorting sorts the bound model straight away. A paged result answers that by
+// re-running its statement wrapped in an ORDER BY, which both reverses the rows
+// and makes the database order the whole result set before the first page can
+// be shown.
+TEST_F(QueryResultPresenterTest, PresentDoesNotSortTheResult) {
+    const auto log = std::make_shared<SortLog>();
+
+    presenter->present({modelWithRows(3, log)});
+
+    EXPECT_FALSE(log->sorted()) << "the result was sorted before anyone asked for an order";
+
+    const auto views = parent->findChildren<QTableView *>();
+    ASSERT_EQ(views.size(), 1);
+    EXPECT_EQ(views.at(0)->horizontalHeader()->sortIndicatorSection(), -1)
+        << "the result opened claiming to be sorted by a column";
+}
+
+TEST_F(QueryResultPresenterTest, PresentToViewDoesNotSortTheTable) {
+    QTableView view(parent.get());
+
+    const auto first = std::make_shared<SortLog>();
+    const auto second = std::make_shared<SortLog>();
+
+    presenter->presentToView(&view, modelWithRows(3, first));
+    presenter->presentToView(&view, modelWithRows(4, second));
+
+    EXPECT_FALSE(first->sorted()) << "the outgoing table was sorted on its way out";
+    EXPECT_FALSE(second->sorted()) << "the table was sorted before anyone asked for an order";
+}
+
+TEST_F(QueryResultPresenterTest, FirstClickOnAColumnSortsAscending) {
+    const auto log = std::make_shared<SortLog>();
+    presenter->present({modelWithRows(3, log)});
+    const auto views = parent->findChildren<QTableView *>();
+    ASSERT_EQ(views.size(), 1);
+
+    clickHeaderSection(views.at(0), 0);
+
+    ASSERT_FALSE(log->orders.isEmpty()) << "clicking the header sorted nothing";
+    EXPECT_EQ(log->columns.last(), 0);
+    EXPECT_EQ(log->orders.last(), Qt::AscendingOrder);
+}
+
+TEST_F(QueryResultPresenterTest, SecondClickOnTheSameColumnSortsDescending) {
+    const auto log = std::make_shared<SortLog>();
+    presenter->present({modelWithRows(3, log)});
+    const auto views = parent->findChildren<QTableView *>();
+    ASSERT_EQ(views.size(), 1);
+
+    clickHeaderSection(views.at(0), 0);
+    clickHeaderSection(views.at(0), 0);
+
+    ASSERT_FALSE(log->orders.isEmpty()) << "clicking the header sorted nothing";
+    EXPECT_EQ(log->orders.last(), Qt::DescendingOrder);
+}