ClangTools: Reflect state of fixits

...in the fixit column to avoid confusion.

As a side effect, add some error handling.

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

src/plugins/clangtools/clangtidyclazytool.cpp

@@ -69,42 +69,102 @@ namespace Internal {
 
 static ClangTidyClazyTool *s_instance;
 
-static void applyFixits(const QVector<Diagnostic> &diagnostics)
+class ApplyFixIts
 {
-    TextEditor::RefactoringChanges changes;
+public:
-    QMap<QString, TextEditor::RefactoringFilePtr> refactoringFiles;
+    class RefactoringFileInfo
+    {
+    public:
+        bool isValid() const { return file; }
 
-    // Create refactoring files and changes
+        TextEditor::RefactoringFilePtr file;
-    for (const Diagnostic &diagnostic : diagnostics) {
+        QVector<DiagnosticItem *> diagnosticItems;
-        const auto filePath = diagnostic.location.filePath;
+        bool hasScheduledFixits = false;
+    };
+
+    ApplyFixIts(const QVector<DiagnosticItem *> &diagnosticItems)
+    {
+        for (DiagnosticItem *diagnosticItem : diagnosticItems) {
+            const QString &filePath = diagnosticItem->diagnostic().location.filePath;
             QTC_ASSERT(!filePath.isEmpty(), continue);
 
             // Get or create refactoring file
-        TextEditor::RefactoringFilePtr refactoringFile = refactoringFiles.value(filePath);
+            RefactoringFileInfo &fileInfo = m_refactoringFileInfos[filePath];
-        if (!refactoringFile) {
+            if (!fileInfo.isValid())
-            refactoringFile = changes.file(filePath);
+                fileInfo.file = m_changes.file(filePath);
-            refactoringFiles.insert(filePath, refactoringFile);
+
+            // Append item
+            fileInfo.diagnosticItems += diagnosticItem;
+            if (diagnosticItem->fixItStatus() == FixitStatus::Scheduled)
+                fileInfo.hasScheduledFixits = true;
+        }
     }
 
-        // Append changes
-        ChangeSet cs = refactoringFile->changeSet();
 
+    static bool addChanges(TextEditor::RefactoringFilePtr file,
+                           const Diagnostic &diagnostic,
+                           ChangeSet &changeSet)
+    {
         for (const ExplainingStep &step : diagnostic.explainingSteps) {
-            if (step.isFixIt) {
+            if (!step.isFixIt)
+                continue;
+
             const Debugger::DiagnosticLocation start = step.ranges.first();
             const Debugger::DiagnosticLocation end = step.ranges.last();
-                cs.replace(refactoringFile->position(start.line, start.column),
+            const bool operationAdded = changeSet.replace(file->position(start.line, start.column),
-                           refactoringFile->position(end.line, end.column), step.message);
+                                                          file->position(end.line, end.column),
+                                                          step.message);
+            if (!operationAdded)
+                return false;
+        }
+
+        return true;
+    }
+
+    void apply()
+    {
+        for (auto i = m_refactoringFileInfos.begin(); i != m_refactoringFileInfos.end(); ++i) {
+            const RefactoringFileInfo &fileInfo = i.value();
+
+            QVector<DiagnosticItem *> itemsSucceeded;
+            QVector<DiagnosticItem *> itemsFailed;
+            QVector<DiagnosticItem *> itemsInvalidated;
+
+            // Construct change set
+            for (DiagnosticItem *diagnosticItem : fileInfo.diagnosticItems) {
+                const FixitStatus fixItStatus = diagnosticItem->fixItStatus();
+                if (fixItStatus == FixitStatus::Scheduled) {
+                    ChangeSet changeSet = fileInfo.file->changeSet();
+                    if (addChanges(fileInfo.file, diagnosticItem->diagnostic(), changeSet))
+                        itemsSucceeded += diagnosticItem;
+                    else // Ops, some fixits might have overlapping ranges.
+                        itemsFailed += diagnosticItem;
+                    fileInfo.file->setChangeSet(changeSet);
+                } else if (fileInfo.hasScheduledFixits && fixItStatus == FixitStatus::NotScheduled) {
+                    itemsInvalidated += diagnosticItem;
                 }
             }
 
-        refactoringFile->setChangeSet(cs);
+            // Apply file
+            if (!fileInfo.file->apply()) {
+                itemsSucceeded.clear();
+                itemsFailed = fileInfo.diagnosticItems;
             }
 
-    // Apply refactoring file changes
+            // Update DiagnosticItem state
-    for (TextEditor::RefactoringFilePtr refactoringFile  : refactoringFiles.values())
+            for (DiagnosticItem *diagnosticItem : itemsSucceeded)
-        refactoringFile->apply();
+                diagnosticItem->setFixItStatus(FixitStatus::Applied);
-}
+            for (DiagnosticItem *diagnosticItem : itemsFailed)
+                diagnosticItem->setFixItStatus(FixitStatus::FailedToApply);
+            for (DiagnosticItem *diagnosticItem : itemsInvalidated)
+                diagnosticItem->setFixItStatus(FixitStatus::Invalidated);
+        }
+    }
+
+private:
+    TextEditor::RefactoringChanges m_changes;
+    QMap<QString, RefactoringFileInfo> m_refactoringFileInfos;
+};
 
 ClangTidyClazyTool::ClangTidyClazyTool()
     : ClangTool("Clang-Tidy and Clazy")
@@ -166,16 +226,12 @@ ClangTidyClazyTool::ClangTidyClazyTool()
             &ClangToolsDiagnosticModel::fixItsToApplyCountChanged,
             [this](int c) { m_applyFixitsButton->setEnabled(c); });
     connect(m_applyFixitsButton, &QToolButton::clicked, [this]() {
-        QVector<Diagnostic> diagnosticsWithFixits;
+        QVector<DiagnosticItem *> diagnosticItems;
+        m_diagnosticModel->rootItem()->forChildrenAtLevel(1, [&](TreeItem *item){
+            diagnosticItems += static_cast<DiagnosticItem *>(item);
+        });
 
-        const int count = m_diagnosticModel->rootItem()->childCount();
+        ApplyFixIts(diagnosticItems).apply();
-        for (int i = 0; i < count; ++i) {
-            auto *item = static_cast<DiagnosticItem *>(m_diagnosticModel->rootItem()->childAt(i));
-            if (item->applyFixits())
-                diagnosticsWithFixits += item->diagnostic();
-        }
-
-        applyFixits(diagnosticsWithFixits);
     });
 
     ActionContainer *menu = ActionManager::actionContainer(Debugger::Constants::M_DEBUG_ANALYZER);

src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp

@@ -55,18 +55,21 @@ private:
 ClangToolsDiagnosticModel::ClangToolsDiagnosticModel(QObject *parent)
     : Utils::TreeModel<>(parent)
 {
-    setHeader({tr("Issue"), tr("Location"), tr("Fixits")});
+    setHeader({tr("Issue"), tr("Location"), tr("Fixit Status")});
 }
 
 void ClangToolsDiagnosticModel::addDiagnostics(const QList<Diagnostic> &diagnostics)
 {
-    const auto onFixItChanged = [this](bool checked){
+    const auto onFixitStatusChanged = [this](FixitStatus newStatus) {
-        m_fixItsToApplyCount += checked ? +1 : -1;
+        if (newStatus == FixitStatus::Scheduled)
+            ++m_fixItsToApplyCount;
+        else
+            --m_fixItsToApplyCount;
         emit fixItsToApplyCountChanged(m_fixItsToApplyCount);
     };
 
     for (const Diagnostic &d : diagnostics)
-        rootItem()->appendChild(new DiagnosticItem(d, onFixItChanged));
+        rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged));
 }
 
 QList<Diagnostic> ClangToolsDiagnosticModel::diagnostics() const
@@ -203,10 +206,13 @@ static QString fullText(const Diagnostic &diagnostic)
 }
 
 
-DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnCheckedFixit &onCheckedFixit)
+DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged)
     : m_diagnostic(diag)
-    , m_onCheckedFixit(onCheckedFixit)
+    , m_onFixitStatusChanged(onFixitStatusChanged)
 {
+    if (diag.hasFixits)
+        m_fixitStatus = FixitStatus::NotScheduled;
+
     // Don't show explaining steps if they add no information.
     if (diag.explainingSteps.count() == 1) {
         const ExplainingStep &step = diag.explainingSteps.first();
@@ -222,10 +228,16 @@ Qt::ItemFlags DiagnosticItem::flags(int column) const
 {
     const Qt::ItemFlags itemFlags = TreeItem::flags(column);
     if (column == DiagnosticView::FixItColumn) {
-        if (m_diagnostic.hasFixits)
+        switch (m_fixitStatus) {
-            return itemFlags | Qt::ItemIsUserCheckable;
+        case FixitStatus::NotAvailable:
-        else
+        case FixitStatus::Applied:
+        case FixitStatus::FailedToApply:
+        case FixitStatus::Invalidated:
             return itemFlags & ~Qt::ItemIsEnabled;
+        case FixitStatus::Scheduled:
+        case FixitStatus::NotScheduled:
+            return itemFlags | Qt::ItemIsUserCheckable;
+        }
     }
 
     return itemFlags;
@@ -250,8 +262,33 @@ QVariant DiagnosticItem::data(int column, int role) const
         return Debugger::DetailedErrorView::locationData(role, m_diagnostic.location);
 
     if (column == DiagnosticView::FixItColumn) {
-        if (role == Qt::CheckStateRole)
+        if (role == Qt::CheckStateRole) {
-            return m_applyFixits ? Qt::Checked : Qt::Unchecked;
+            switch (m_fixitStatus) {
+            case FixitStatus::NotAvailable:
+            case FixitStatus::NotScheduled:
+            case FixitStatus::Invalidated:
+            case FixitStatus::Applied:
+            case FixitStatus::FailedToApply:
+                return Qt::Unchecked;
+            case FixitStatus::Scheduled:
+                return Qt::Checked;
+            }
+        } else if (role == Qt::DisplayRole) {
+            switch (m_fixitStatus) {
+            case FixitStatus::NotAvailable:
+                return ClangToolsDiagnosticModel::tr("No Fixits");
+            case FixitStatus::NotScheduled:
+                return ClangToolsDiagnosticModel::tr("Not Scheduled");
+            case FixitStatus::Invalidated:
+                return ClangToolsDiagnosticModel::tr("Invalidated");
+            case FixitStatus::Scheduled:
+                return ClangToolsDiagnosticModel::tr("Scheduled");
+            case FixitStatus::FailedToApply:
+                return ClangToolsDiagnosticModel::tr("Failed to Apply");
+            case FixitStatus::Applied:
+                return ClangToolsDiagnosticModel::tr("Applied");
+            }
+        }
         return QVariant();
     }
 
@@ -275,16 +312,31 @@ QVariant DiagnosticItem::data(int column, int role) const
 bool DiagnosticItem::setData(int column, const QVariant &data, int role)
 {
     if (column == DiagnosticView::FixItColumn && role == Qt::CheckStateRole) {
-        m_applyFixits = data.value<Qt::CheckState>() == Qt::Checked ? true : false;
+        const FixitStatus newStatus = data.value<Qt::CheckState>() == Qt::Checked
-        update();
+                                          ? FixitStatus::Scheduled
-        if (m_onCheckedFixit)
+                                          : FixitStatus::NotScheduled;
-            m_onCheckedFixit(m_applyFixits);
+
+        setFixItStatus(newStatus);
         return true;
     }
 
     return Utils::TreeItem::setData(column, data, role);
 }
 
+void DiagnosticItem::setFixItStatus(const FixitStatus &status)
+{
+    const FixitStatus oldStatus = m_fixitStatus;
+    m_fixitStatus = status;
+    update();
+    if (m_onFixitStatusChanged && status != oldStatus)
+        m_onFixitStatusChanged(status);
+}
+
+FixitStatus DiagnosticItem::fixItStatus() const
+{
+    return m_fixitStatus;
+}
+
 ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step) : m_step(step)
 {
 }

src/plugins/clangtools/clangtoolsdiagnosticmodel.h

@@ -42,14 +42,25 @@ namespace ProjectExplorer { class Project; }
 namespace ClangTools {
 namespace Internal {
 
+enum class FixitStatus {
+    NotAvailable,
+    NotScheduled,
+    Scheduled,
+    Applied,
+    FailedToApply,
+    Invalidated,
+};
+
 class DiagnosticItem : public Utils::TreeItem
 {
 public:
-    using OnCheckedFixit = std::function<void(bool)>;
+    using OnFixitStatusChanged = std::function<void(FixitStatus newStatus)>;
-    DiagnosticItem(const Diagnostic &diag, const OnCheckedFixit &onCheckedFixit);
+    DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged);
 
     Diagnostic diagnostic() const { return m_diagnostic; }
-    bool applyFixits() const { return m_applyFixits; }
+
+    FixitStatus fixItStatus() const;
+    void setFixItStatus(const FixitStatus &status);
 
 private:
     Qt::ItemFlags flags(int column) const override;
@@ -58,8 +69,8 @@ private:
 
 private:
     const Diagnostic m_diagnostic;
-    bool m_applyFixits = false;
+    FixitStatus m_fixitStatus = FixitStatus::NotAvailable;
-    OnCheckedFixit m_onCheckedFixit;
+    OnFixitStatusChanged m_onFixitStatusChanged;
 };
 
 class ClangToolsDiagnosticModel : public Utils::TreeModel<>

src/plugins/texteditor/refactoringchanges.cpp

@@ -321,7 +321,7 @@ void RefactoringFile::setOpenEditor(bool activate, int pos)
     m_editorCursorPosition = pos;
 }
 
-void RefactoringFile::apply()
+bool RefactoringFile::apply()
 {
     // test file permissions
     if (!QFileInfo(fileName()).isWritable()) {
@@ -331,7 +331,7 @@ void RefactoringFile::apply()
                                                                 "Refactoring cannot be applied.");
         roDialog.setShowFailWarning(true, failDetailText);
         if (roDialog.exec() == ReadOnlyFilesDialog::RO_Cancel)
-            return;
+            return false;
     }
 
     // open / activate / goto position
@@ -345,6 +345,8 @@ void RefactoringFile::apply()
         m_editorCursorPosition = -1;
     }
 
+    bool result = true;
+
     // apply changes, if any
     if (m_data && !(m_indentRanges.isEmpty() && m_changes.isEmpty())) {
         QTextDocument *doc = mutableDocument();
@@ -374,10 +376,12 @@ void RefactoringFile::apply()
 
             // if this document doesn't have an editor, write the result to a file
             if (!m_editor && m_textFileFormat.codec) {
-                QTC_ASSERT(!m_fileName.isEmpty(), return);
+                QTC_ASSERT(!m_fileName.isEmpty(), return false);
                 QString error;
-                if (!m_textFileFormat.writeFile(m_fileName, doc->toPlainText(), &error))
+                if (!m_textFileFormat.writeFile(m_fileName, doc->toPlainText(), &error)) {
                     qWarning() << "Could not apply changes to" << m_fileName << ". Error: " << error;
+                    result = false;
+                }
             }
 
             fileChanged();
@@ -385,6 +389,7 @@ void RefactoringFile::apply()
     }
 
     m_appliedOnce = true;
+    return result;
 }
 
 void RefactoringFile::indentOrReindent(void (RefactoringChangesData::*mf)(const QTextCursor &,

src/plugins/texteditor/refactoringchanges.h

@@ -79,7 +79,7 @@ public:
     void appendIndentRange(const Range &range);
     void appendReindentRange(const Range &range);
     void setOpenEditor(bool activate = false, int pos = -1);
-    void apply();
+    bool apply();
 
 protected:
     // users may only get const access to RefactoringFiles created through