From d122049aeea62794ba97dd60516cf97c2cdaaffa Mon Sep 17 00:00:00 2001
From: Nikolai Kosjar <nikolai.kosjar@qt.io>
Date: Thu, 7 Feb 2019 09:29:15 +0100
Subject: [PATCH] ClangTools: Fix order of ExplainingStepItems

We sorted by line/column/text, but we should not change the order for
these items as the original order is crucial to understand the
diagnostics. For example, the clang static analyzer diagnostics provide
step by step notes to explain the main diagnostics and these does not
match the line/column order.

Change-Id: I1e7235b37eb5713b0b7135aab46124f590a1443a
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>
---
 .../clangtools/clangtoolsdiagnosticmodel.cpp  | 46 ++++++++++++-------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp b/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp
index 7a2d78ed3a3..6e16cc6eec5 100644
--- a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp
+++ b/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp
@@ -70,12 +70,14 @@ QVariant FilePathItem::data(int column, int role) const
 class ExplainingStepItem : public Utils::TreeItem
 {
 public:
-    ExplainingStepItem(const ExplainingStep &step);
+    ExplainingStepItem(const ExplainingStep &step, int index);
+    int index() const { return m_index; }
 
 private:
     QVariant data(int column, int role) const override;
 
     const ExplainingStep m_step;
+    const int m_index = 0;
 };
 
 ClangToolsDiagnosticModel::ClangToolsDiagnosticModel(QObject *parent)
@@ -350,8 +352,8 @@ DiagnosticItem::DiagnosticItem(const Diagnostic &diag,
     if (!diag.explainingSteps.isEmpty())
         m_parentModel->stepsToItemsCache[diag.explainingSteps].push_back(this);
 
-    foreach (const ExplainingStep &s, diag.explainingSteps)
-        appendChild(new ExplainingStepItem(s));
+    for (int i = 0; i < diag.explainingSteps.size(); ++i )
+        appendChild(new ExplainingStepItem(diag.explainingSteps[i], i));
 }
 
 DiagnosticItem::~DiagnosticItem()
@@ -487,9 +489,10 @@ bool DiagnosticItem::hasNewFixIts() const
     return m_parentModel->stepsToItemsCache[m_diagnostic.explainingSteps].front() == this;
 }
 
-ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step) : m_step(step)
-{
-}
+ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step, int index)
+    : m_step(step)
+    , m_index(index)
+{}
 
 // We expect something like "note: ..."
 static QVariant iconForExplainingStepMessage(const QString &message)
@@ -561,7 +564,6 @@ QVariant ExplainingStepItem::data(int column, int role) const
     return QVariant();
 }
 
-
 DiagnosticFilterModel::DiagnosticFilterModel(QObject *parent)
     : QSortFilterProxyModel(parent)
 {
@@ -655,22 +657,34 @@ bool DiagnosticFilterModel::lessThan(const QModelIndex &l, const QModelIndex &r)
     const bool isComparingDiagnostics = !dynamic_cast<FilePathItem *>(itemLeft);
 
     if (sortColumn() == Debugger::DetailedErrorView::DiagnosticColumn && isComparingDiagnostics) {
-        using Debugger::DiagnosticLocation;
-        const int role = Debugger::DetailedErrorView::LocationRole;
+        bool result = false;
+        if (dynamic_cast<DiagnosticItem *>(itemLeft)) {
+            using Debugger::DiagnosticLocation;
+            const int role = Debugger::DetailedErrorView::LocationRole;
 
-        const auto leftLoc = sourceModel()->data(l, role).value<DiagnosticLocation>();
-        const auto leftText = sourceModel()->data(l, ClangToolsDiagnosticModel::TextRole).toString();
+            const auto leftLoc = sourceModel()->data(l, role).value<DiagnosticLocation>();
+            const auto leftText
+                = sourceModel()->data(l, ClangToolsDiagnosticModel::TextRole).toString();
 
-        const auto rightLoc = sourceModel()->data(r, role).value<DiagnosticLocation>();
-        const auto rightText = sourceModel()->data(r, ClangToolsDiagnosticModel::TextRole).toString();
+            const auto rightLoc = sourceModel()->data(r, role).value<DiagnosticLocation>();
+            const auto rightText
+                = sourceModel()->data(r, ClangToolsDiagnosticModel::TextRole).toString();
+
+            result = std::tie(leftLoc.line, leftLoc.column, leftText)
+                     < std::tie(rightLoc.line, rightLoc.column, rightText);
+        } else if (auto left = dynamic_cast<ExplainingStepItem *>(itemLeft)) {
+            const auto right = dynamic_cast<ExplainingStepItem *>(model->itemForIndex(r));
+            result = left->index() < right->index();
+        } else {
+            QTC_CHECK(false && "Unexpected item");
+        }
 
-        const int result = std::tie(leftLoc.line, leftLoc.column, leftText)
-                           < std::tie(rightLoc.line, rightLoc.column, rightText);
         if (sortOrder() == Qt::DescendingOrder)
-            return !result; // Ensure that we always sort location from top to bottom.
+            return !result; // Do not change the order of these item as this might be confusing.
         return result;
     }
 
+    // FilePathItem
     return QSortFilterProxyModel::lessThan(l, r);
 }