ClangTools: Organize diagnostics by file path

* Introduce the file path as a top level node.
* Remove the location column.
  * Encode the line/column information in the DisplayRole, as for the
    Clang Code Model tooltips.
  * Double click on a diagnostic opens the editor.

Change-Id: I4c263537cc04c3c4feb6ccd5c395d60d8bee0bc3
Reviewed-by: Ivan Donchevskii <ivan.donchevskii@qt.io>

src/plugins/clangtools/clangtidyclazytool.cpp

@@ -216,7 +216,8 @@ ClangTidyClazyTool::ClangTidyClazyTool()
     initDiagnosticView();
     m_diagnosticView->setModel(m_diagnosticFilterModel);
     m_diagnosticView->setSortingEnabled(true);
-    m_diagnosticView->sortByColumn(Debugger::DetailedErrorView::LocationColumn, Qt::AscendingOrder);
+    m_diagnosticView->sortByColumn(Debugger::DetailedErrorView::DiagnosticColumn,
+                                   Qt::AscendingOrder);
     m_diagnosticView->setObjectName(QLatin1String("ClangTidyClazyIssuesView"));
     m_diagnosticView->setWindowTitle(tr("Clang-Tidy and Clazy Issues"));
 
@@ -299,7 +300,7 @@ ClangTidyClazyTool::ClangTidyClazyTool()
     });
     connect(m_applyFixitsButton, &QToolButton::clicked, [this]() {
         QVector<DiagnosticItem *> diagnosticItems;
-        m_diagnosticModel->rootItem()->forChildrenAtLevel(1, [&](TreeItem *item){
+        m_diagnosticModel->rootItem()->forChildrenAtLevel(2, [&](TreeItem *item){
             diagnosticItems += static_cast<DiagnosticItem *>(item);
         });
 

src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp

@@ -29,6 +29,7 @@
 #include "clangtoolsprojectsettings.h"
 #include "clangtoolsutils.h"
 
+#include <coreplugin/fileiconprovider.h>
 #include <projectexplorer/project.h>
 #include <projectexplorer/session.h>
 #include <utils/qtcassert.h>
@@ -37,11 +38,33 @@
 #include <QFileInfo>
 #include <QLoggingCategory>
 
+#include <tuple>
+
 static Q_LOGGING_CATEGORY(LOG, "qtc.clangtools.model", QtWarningMsg)
 
 namespace ClangTools {
 namespace Internal {
 
+FilePathItem::FilePathItem(const QString &filePath)
+    : m_filePath(filePath)
+{}
+
+QVariant FilePathItem::data(int column, int role) const
+{
+    if (column == DiagnosticView::DiagnosticColumn) {
+        switch (role) {
+        case Qt::DisplayRole:
+            return m_filePath;
+        case Qt::DecorationRole:
+            return Core::FileIconProvider::icon(m_filePath);
+        default:
+            return QVariant();
+        }
+    }
+
+    return QVariant();
+}
+
 class ExplainingStepItem : public Utils::TreeItem
 {
 public:
@@ -57,7 +80,7 @@ ClangToolsDiagnosticModel::ClangToolsDiagnosticModel(QObject *parent)
     : Utils::TreeModel<>(parent)
     , m_filesWatcher(std::make_unique<QFileSystemWatcher>())
 {
-    setHeader({tr("Issue"), tr("Location"), tr("Fixit Status")});
+    setHeader({tr("Issue"), tr("Fixit Status")});
     connectFileWatcher();
 }
 
@@ -85,6 +108,7 @@ void ClangToolsDiagnosticModel::addDiagnostics(const QList<Diagnostic> &diagnost
     };
 
     for (const Diagnostic &d : diagnostics) {
+        // Check for duplicates
         const int previousItemCount = m_diagnostics.count();
         m_diagnostics.insert(d);
         if (m_diagnostics.count() == previousItemCount) {
@@ -92,9 +116,19 @@ void ClangToolsDiagnosticModel::addDiagnostics(const QList<Diagnostic> &diagnost
             continue;
         }
 
+        // Create file path item if necessary
+        const QString filePath = d.location.filePath;
+        FilePathItem *&filePathItem = m_filePathToItem[filePath];
+        if (!filePathItem) {
+            filePathItem = new FilePathItem(filePath);
+            rootItem()->appendChild(filePathItem);
+
+            addWatchedPath(d.location.filePath);
+        }
+
+        // Add to file path item
         qCDebug(LOG) << "Adding diagnostic:" << d;
-        addWatchedPath(d.location.filePath);
+        filePathItem->appendChild(new DiagnosticItem(d, onFixitStatusChanged, this));
-        rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged, this));
     }
 }
 
@@ -105,6 +139,7 @@ QSet<Diagnostic> ClangToolsDiagnosticModel::diagnostics() const
 
 void ClangToolsDiagnosticModel::clear()
 {
+    m_filePathToItem.clear();
     m_diagnostics.clear();
     clearAndSetupCache();
     Utils::TreeModel<>::clear();
@@ -135,11 +170,11 @@ void ClangToolsDiagnosticModel::clearAndSetupCache()
 
 void ClangToolsDiagnosticModel::onFileChanged(const QString &path)
 {
-    for (Utils::TreeItem * const item : *rootItem()) {
+    rootItem()->forChildrenAtLevel(2, [&](Utils::TreeItem *item){
         auto diagnosticItem = static_cast<DiagnosticItem *>(item);
         if (diagnosticItem->diagnostic().location.filePath == path)
             diagnosticItem->setFixItStatus(FixitStatus::Invalidated);
-    }
+    });
     removeWatchedPath(path);
 }
 
@@ -345,11 +380,15 @@ static QVariant iconData(const QString &type)
     return QVariant();
 }
 
+static QString withLineColumnPrefixed(const QString &text,
+                                      const Debugger::DiagnosticLocation &location)
+{
+    return QString("%1:%2: %3")
+        .arg(QString::number(location.line), QString::number(location.column), text);
+}
+
 QVariant DiagnosticItem::data(int column, int role) const
 {
-    if (column == Debugger::DetailedErrorView::LocationColumn)
-        return Debugger::DetailedErrorView::locationData(role, m_diagnostic.location);
-
     if (column == DiagnosticView::FixItColumn) {
         if (role == Qt::CheckStateRole) {
             switch (m_fixitStatus) {
@@ -378,24 +417,28 @@ QVariant DiagnosticItem::data(int column, int role) const
                 return ClangToolsDiagnosticModel::tr("Applied");
             }
         }
-        return QVariant();
+    } else if (column == DiagnosticView::DiagnosticColumn) {
+        switch (role) {
+        case Debugger::DetailedErrorView::LocationRole:
+            return QVariant::fromValue(m_diagnostic.location);
+        case Debugger::DetailedErrorView::FullTextRole:
+            return fullText(m_diagnostic);
+        case ClangToolsDiagnosticModel::DiagnosticRole:
+            return QVariant::fromValue(m_diagnostic);
+        case ClangToolsDiagnosticModel::TextRole:
+            return m_diagnostic.description;
+        case Qt::DisplayRole:
+            return withLineColumnPrefixed(m_diagnostic.description, m_diagnostic.location);
+        case Qt::ToolTipRole:
+            return createDiagnosticToolTipString(m_diagnostic);
+        case Qt::DecorationRole:
+            return iconData(m_diagnostic.type);
+        default:
+            return QVariant();
+        }
     }
 
-    // DiagnosticColumn
+    return QVariant();
-    switch (role) {
-    case Debugger::DetailedErrorView::FullTextRole:
-        return fullText(m_diagnostic);
-    case ClangToolsDiagnosticModel::DiagnosticRole:
-        return QVariant::fromValue(m_diagnostic);
-    case Qt::DisplayRole:
-        return m_diagnostic.description;
-    case Qt::ToolTipRole:
-        return createDiagnosticToolTipString(m_diagnostic);
-    case Qt::DecorationRole:
-        return iconData(m_diagnostic.type);
-    default:
-        return QVariant();
-    }
 }
 
 bool DiagnosticItem::setData(int column, const QVariant &data, int role)
@@ -454,27 +497,32 @@ static QVariant iconForExplainingStepMessage(const QString &message)
 
 QVariant ExplainingStepItem::data(int column, int role) const
 {
-    if (column == Debugger::DetailedErrorView::LocationColumn)
-        return Debugger::DetailedErrorView::locationData(role, m_step.location);
-
     if (column == DiagnosticView::FixItColumn)
         return QVariant();
 
-    // DiagnosticColumn
+    if (column == DiagnosticView::DiagnosticColumn) {
-    switch (role) {
+        // DiagnosticColumn
-    case Debugger::DetailedErrorView::FullTextRole:
+        switch (role) {
-        return fullText(static_cast<DiagnosticItem *>(parent())->diagnostic());
+        case Debugger::DetailedErrorView::LocationRole:
-    case ClangToolsDiagnosticModel::DiagnosticRole:
+            return QVariant::fromValue(m_step.location);
-        return QVariant::fromValue(static_cast<DiagnosticItem *>(parent())->diagnostic());
+        case Debugger::DetailedErrorView::FullTextRole:
-    case Qt::DisplayRole:
+            return fullText(static_cast<DiagnosticItem *>(parent())->diagnostic());
-        return m_step.message;
+        case ClangToolsDiagnosticModel::TextRole:
-    case Qt::ToolTipRole:
+            return m_step.message;
-        return createExplainingStepToolTipString(m_step);
+        case ClangToolsDiagnosticModel::DiagnosticRole:
-    case Qt::DecorationRole:
+            return QVariant::fromValue(static_cast<DiagnosticItem *>(parent())->diagnostic());
-        return iconForExplainingStepMessage(m_step.message);
+        case Qt::DisplayRole:
-    default:
+            return m_step.message;
-        return QVariant();
+        case Qt::ToolTipRole:
+            return createExplainingStepToolTipString(m_step);
+        case Qt::DecorationRole:
+            return iconForExplainingStepMessage(m_step.message);
+        default:
+            return QVariant();
+        }
     }
+
+    return QVariant();
 }
 
 
@@ -518,30 +566,58 @@ void DiagnosticFilterModel::addSuppressedDiagnostic(
 bool DiagnosticFilterModel::filterAcceptsRow(int sourceRow,
         const QModelIndex &sourceParent) const
 {
-    // Avoid filtering child diagnostics / explaining steps.
-    if (sourceParent.isValid())
-        return true;
-
-    // Is the diagnostic suppressed?
     auto model = static_cast<ClangToolsDiagnosticModel *>(sourceModel());
-    auto item = static_cast<DiagnosticItem *>(model->rootItem()->childAt(sourceRow));
+    Utils::TreeItem *item = model->itemForIndex(sourceParent);
-    const Diagnostic &diag = item->diagnostic();
+
-    foreach (const SuppressedDiagnostic &d, m_suppressedDiagnostics) {
+    // DiagnosticItem
-        if (d.description != diag.description)
+    if (auto filePathItem = dynamic_cast<FilePathItem *>(item)) {
-            continue;
+        auto diagnosticItem = dynamic_cast<DiagnosticItem *>(filePathItem->childAt(sourceRow));
-        QString filePath = d.filePath.toString();
+        QTC_ASSERT(diagnosticItem, return false);
-        QFileInfo fi(filePath);
+
-        if (fi.isRelative())
+        // Is the diagnostic explicitly suppressed?
-            filePath = m_lastProjectDirectory.toString() + QLatin1Char('/') + filePath;
+        const Diagnostic &diag = diagnosticItem->diagnostic();
-        if (filePath == diag.location.filePath)
+        foreach (const SuppressedDiagnostic &d, m_suppressedDiagnostics) {
-            return false;
+            if (d.description != diag.description)
+                continue;
+            QString filePath = d.filePath.toString();
+            QFileInfo fi(filePath);
+            if (fi.isRelative())
+                filePath = m_lastProjectDirectory.toString() + QLatin1Char('/') + filePath;
+            if (filePath == diag.location.filePath)
+                return false;
+        }
+
+        // Does the diagnostic match the filter?
+        return diag.description.contains(filterRegExp());
     }
 
-    // Does the diagnostic match the filter?
+    return true;
-    if (diag.description.contains(filterRegExp()))
+}
-        return true;
 
-    return false;
+bool DiagnosticFilterModel::lessThan(const QModelIndex &l, const QModelIndex &r) const
+{
+    auto model = static_cast<ClangToolsDiagnosticModel *>(sourceModel());
+    Utils::TreeItem *itemLeft = model->itemForIndex(l);
+    const bool isComparingDiagnostics = !dynamic_cast<FilePathItem *>(itemLeft);
+
+    if (sortColumn() == Debugger::DetailedErrorView::DiagnosticColumn && isComparingDiagnostics) {
+        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 rightLoc = sourceModel()->data(r, role).value<DiagnosticLocation>();
+        const auto rightText = sourceModel()->data(r, ClangToolsDiagnosticModel::TextRole).toString();
+
+        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;
+    }
+
+    return QSortFilterProxyModel::lessThan(l, r);
 }
 
 void DiagnosticFilterModel::handleSuppressedDiagnosticsChanged()

src/plugins/clangtools/clangtoolsdiagnosticmodel.h

@@ -58,6 +58,16 @@ enum class FixitStatus {
 
 class ClangToolsDiagnosticModel;
 
+class FilePathItem : public Utils::TreeItem
+{
+public:
+    FilePathItem(const QString &filePath);
+    QVariant data(int column, int role) const override;
+
+private:
+    const QString m_filePath;
+};
+
 class DiagnosticItem : public Utils::TreeItem
 {
 public:
@@ -75,10 +85,11 @@ public:
     ReplacementOperations &fixitOperations() { return m_fixitOperations; }
     void setFixitOperations(const ReplacementOperations &replacements);
 
+    bool setData(int column, const QVariant &data, int role) override;
+
 private:
     Qt::ItemFlags flags(int column) const override;
     QVariant data(int column, int role) const override;
-    bool setData(int column, const QVariant &data, int role) override;
 
 private:
     const Diagnostic m_diagnostic;
@@ -101,9 +112,7 @@ public:
     void addDiagnostics(const QList<Diagnostic> &diagnostics);
     QSet<Diagnostic> diagnostics() const;
 
-    enum ItemRole {
+    enum ItemRole { DiagnosticRole = Debugger::DetailedErrorView::FullTextRole + 1, TextRole };
-        DiagnosticRole = Debugger::DetailedErrorView::FullTextRole + 1
-    };
 
     void clear();
     void removeWatchedPath(const QString &path);
@@ -119,6 +128,7 @@ private:
     void clearAndSetupCache();
 
 private:
+    QHash<QString, FilePathItem *> m_filePathToItem;
     QSet<Diagnostic> m_diagnostics;
     std::map<QVector<ExplainingStep>, QVector<DiagnosticItem *>> stepsToItemsCache;
     std::unique_ptr<QFileSystemWatcher> m_filesWatcher;
@@ -138,6 +148,7 @@ public:
 
 private:
     bool filterAcceptsRow(int sourceRow, const QModelIndex &sourceParent) const override;
+    bool lessThan(const QModelIndex &l, const QModelIndex &r) const override;
     void handleSuppressedDiagnosticsChanged();
 
     QPointer<ProjectExplorer::Project> m_project;

src/plugins/clangtools/clangtoolsdiagnosticview.cpp

@@ -29,6 +29,8 @@
 #include "clangtoolsprojectsettings.h"
 #include "clangtoolsutils.h"
 
+#include <coreplugin/editormanager/editormanager.h>
+
 #include <utils/fileutils.h>
 #include <utils/qtcassert.h>
 
@@ -136,6 +138,51 @@ void DiagnosticView::suppressCurrentDiagnostic()
     }
 }
 
+void DiagnosticView::goNext()
+{
+    const QModelIndex currentIndex = selectionModel()->currentIndex();
+    selectIndex(getIndex(currentIndex, Next));
+}
+
+void DiagnosticView::goBack()
+{
+    const QModelIndex currentIndex = selectionModel()->currentIndex();
+    selectIndex(getIndex(currentIndex, Previous));
+}
+
+QModelIndex DiagnosticView::getIndex(const QModelIndex &index, Direction direction) const
+{
+    QModelIndex parentIndex = index.parent();
+
+    // Use direct sibling for level 2 and 3 items is possible
+    if (parentIndex.isValid()) {
+        const QModelIndex nextIndex = index.sibling(index.row() + direction, index.column());
+        if (nextIndex.isValid())
+            return nextIndex;
+    }
+
+    // Last level 3 item? Continue on level 2 item
+    if (parentIndex.parent().isValid())
+        return getIndex(parentIndex, direction);
+
+    // Find next/previous level 2 item
+    QModelIndex nextTopIndex = getTopLevelIndex(parentIndex.isValid() ? parentIndex : index,
+                                                direction);
+    while (!model()->hasChildren(nextTopIndex))
+        nextTopIndex = getTopLevelIndex(nextTopIndex, direction);
+    return model()->index(direction == Next ? 0 : model()->rowCount(nextTopIndex) - 1,
+                          0,
+                          nextTopIndex);
+}
+
+QModelIndex DiagnosticView::getTopLevelIndex(const QModelIndex &index, Direction direction) const
+{
+    QModelIndex below = index.sibling(index.row() + direction, 0);
+    if (below.isValid())
+        return below;
+    return model()->index(direction == Next ? 0 : model()->rowCount(index) - 1, 0);
+}
+
 QList<QAction *> DiagnosticView::customActions() const
 {
     return {m_suppressAction};
@@ -150,11 +197,7 @@ bool DiagnosticView::eventFilter(QObject *watched, QEvent *event)
         case Qt::Key_Return:
         case Qt::Key_Enter:
         case Qt::Key_Space:
-            const QModelIndex current = currentIndex();
+            openEditorForCurrentIndex();
-            const QModelIndex location = model()->index(current.row(),
-                                                        LocationColumn,
-                                                        current.parent());
-            emit clicked(location);
         }
         return true;
     }
@@ -163,6 +206,12 @@ bool DiagnosticView::eventFilter(QObject *watched, QEvent *event)
     }
 }
 
+void DiagnosticView::mouseDoubleClickEvent(QMouseEvent *event)
+{
+    openEditorForCurrentIndex();
+    Debugger::DetailedErrorView::mouseDoubleClickEvent(event);
+}
+
 void DiagnosticView::setSelectedFixItsCount(int fixItsCount)
 {
     if (m_ignoreSetSelectedFixItsCount)
@@ -174,27 +223,35 @@ void DiagnosticView::setSelectedFixItsCount(int fixItsCount)
     clickableFixItHeader->viewport()->update();
 }
 
+void DiagnosticView::openEditorForCurrentIndex()
+{
+    const QVariant v = model()->data(currentIndex(), Debugger::DetailedErrorView::LocationRole);
+    const auto loc = v.value<Debugger::DiagnosticLocation>();
+    if (loc.isValid())
+        Core::EditorManager::openEditorAt(loc.filePath, loc.line, loc.column - 1);
+}
+
 void DiagnosticView::setModel(QAbstractItemModel *theProxyModel)
 {
     const auto proxyModel = static_cast<QSortFilterProxyModel *>(theProxyModel);
-    QAbstractItemModel *sourceModel = proxyModel->sourceModel();
+    const auto sourceModel = static_cast<ClangToolsDiagnosticModel *>(proxyModel->sourceModel());
 
     Debugger::DetailedErrorView::setModel(proxyModel);
     auto *clickableFixItHeader = new ClickableFixItHeader(Qt::Horizontal, this);
-    connect(clickableFixItHeader, &ClickableFixItHeader::fixItColumnClicked,
+    connect(clickableFixItHeader, &ClickableFixItHeader::fixItColumnClicked, this, [=](bool checked) {
-            this, [=](bool checked) {
         m_ignoreSetSelectedFixItsCount = true;
-        for (int row = 0; row < sourceModel->rowCount(); ++row) {
+        sourceModel->rootItem()->forChildrenAtLevel(2, [&](::Utils::TreeItem *item) {
-            QModelIndex index = sourceModel->index(row, FixItColumn, QModelIndex());
+            auto diagnosticItem = static_cast<DiagnosticItem *>(item);
-            sourceModel->setData(index, checked ? Qt::Checked : Qt::Unchecked, Qt::CheckStateRole);
+            diagnosticItem->setData(FixItColumn,
-        }
+                                    checked ? Qt::Checked : Qt::Unchecked,
+                                    Qt::CheckStateRole);
+        });
         m_ignoreSetSelectedFixItsCount = false;
     });
     setHeader(clickableFixItHeader);
     clickableFixItHeader->setStretchLastSection(false);
     clickableFixItHeader->setSectionResizeMode(0, QHeaderView::Stretch);
     clickableFixItHeader->setSectionResizeMode(1, QHeaderView::ResizeToContents);
-    clickableFixItHeader->setSectionResizeMode(2, QHeaderView::ResizeToContents);
 
     const int fixitColumnWidth = clickableFixItHeader->sectionSizeHint(DiagnosticView::FixItColumn);
     const int checkboxWidth = clickableFixItHeader->height();

src/plugins/clangtools/clangtoolsdiagnosticview.h

@@ -38,16 +38,24 @@ public:
     DiagnosticView(QWidget *parent = nullptr);
 
     enum ExtraColumn {
-        FixItColumn = LocationColumn + 1,
+        FixItColumn = DiagnosticColumn + 1,
     };
 
     void setSelectedFixItsCount(int fixItsCount);
 
 private:
+    void openEditorForCurrentIndex();
     void suppressCurrentDiagnostic();
 
+    void goNext() override;
+    void goBack() override;
+    enum Direction { Next = 1, Previous = -1 };
+    QModelIndex getIndex(const QModelIndex &index, Direction direction) const;
+    QModelIndex getTopLevelIndex(const QModelIndex &index, Direction direction) const;
+
     QList<QAction *> customActions() const override;
     bool eventFilter(QObject *watched, QEvent *event) override;
+    void mouseDoubleClickEvent(QMouseEvent *event) override;
     void setModel(QAbstractItemModel *theProxyModel) override;
 
     QAction *m_suppressAction;

src/plugins/debugger/analyzer/detailederrorview.cpp

@@ -108,6 +108,13 @@ void DetailedErrorView::goBack()
     setCurrentRow(prevRow >= 0 ? prevRow : rowCount() - 1);
 }
 
+void DetailedErrorView::selectIndex(const QModelIndex &index)
+{
+    selectionModel()->setCurrentIndex(index,
+                                      QItemSelectionModel::ClearAndSelect
+                                          | QItemSelectionModel::Rows);
+}
+
 QVariant DetailedErrorView::locationData(int role, const DiagnosticLocation &location)
 {
     switch (role) {
@@ -158,9 +165,7 @@ int DetailedErrorView::currentRow() const
 
 void DetailedErrorView::setCurrentRow(int row)
 {
-    const QModelIndex index = model()->index(row, 0);
+    selectIndex(model()->index(row, 0));
-    selectionModel()->setCurrentIndex(index,
-            QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows);
 }
 
 } // namespace Debugger

src/plugins/debugger/analyzer/detailederrorview.h

@@ -42,8 +42,10 @@ public:
     DetailedErrorView(QWidget *parent = nullptr);
     ~DetailedErrorView() override;
 
-    void goNext();
+    virtual void goNext();
-    void goBack();
+    virtual void goBack();
+
+    void selectIndex(const QModelIndex &index);
 
     enum ItemRole {
         LocationRole = Qt::UserRole,