Clang: Clean up diagnostic widget

* Use a single QLabel - No need for all the QLabels we used. Also, a
  single QLabel enables selecting and copying all the diagnostics, which
  is handy when displayed in the info bar.
* Avoid call to Utils::ToolTip::hideImmediately() if the location is
  clicked from the info bar.
* Simplify code and API

Change-Id: Ib991364e4d6f40ef02dada8ebbb90fe6ff8ae1a1
Reviewed-by: Friedemann Kleint <Friedemann.Kleint@qt.io>
Reviewed-by: David Schulz <david.schulz@qt.io>

src/libs/clangbackendipc/diagnosticcontainer.h

@@ -144,6 +144,11 @@ public:
             && first.location_ == second.location_;
     }
 
+    friend bool operator!=(const DiagnosticContainer &first, const DiagnosticContainer &second)
+    {
+        return !(first == second);
+    }
+
 private:
     SourceLocationContainer location_;
     QVector<SourceRangeContainer> ranges_;

src/plugins/clangcodemodel/clangdiagnostictooltipwidget.cpp

@@ -28,34 +28,22 @@
 
 #include <coreplugin/editormanager/editormanager.h>
 
+#include <utils/qtcassert.h>
 #include <utils/tooltip/tooltip.h>
 
+#include <QApplication>
+#include <QDesktopWidget>
 #include <QFileInfo>
-#include <QHBoxLayout>
+#include <QHash>
 #include <QLabel>
-#include <QPushButton>
+
-#include <QVBoxLayout>
+using namespace ClangCodeModel;
+using Internal::ClangDiagnosticWidget;
 
 namespace {
 
 const char LINK_ACTION_GOTO_LOCATION[] = "#gotoLocation";
 const char LINK_ACTION_APPLY_FIX[] = "#applyFix";
-const int childIndentationOnTheLeftInPixel = 10;
-
-QString wrapInBoldTags(const QString &text)
-{
-    return QStringLiteral("<b>") + text + QStringLiteral("</b>");
-}
-
-QString wrapInLink(const QString &text, const QString &target)
-{
-    return QStringLiteral("<a href='%1' style='text-decoration:none'>%2</a>").arg(target, text);
-}
-
-QString wrapInColor(const QString &text, const QByteArray &color)
-{
-    return QStringLiteral("<font color='%2'>%1</font>").arg(text, QString::fromUtf8(color));
-}
 
 QString fileNamePrefix(const QString &mainFilePath,
                        const ClangBackEnd::SourceLocationContainer &location)
@@ -74,181 +62,293 @@ QString locationToString(const ClangBackEnd::SourceLocationContainer &location)
          + QString::number(location.column());
 }
 
-QString clickableLocation(const QString &mainFilePath,
+void openEditorAt(const ClangBackEnd::DiagnosticContainer &diagnostic)
-                          const ClangBackEnd::SourceLocationContainer &location)
 {
-    const QString filePrefix = fileNamePrefix(mainFilePath, location);
+    const ClangBackEnd::SourceLocationContainer location = diagnostic.location();
-    const QString lineColumn = locationToString(location);
-    const QString linkText = filePrefix + lineColumn;
 
-    return wrapInLink(linkText, QLatin1String(LINK_ACTION_GOTO_LOCATION));
-}
-
-QString clickableFixIt(const QString &text, bool hasFixIt)
-{
-    if (!hasFixIt)
-        return text;
-
-    QString clickableText = text;
-    QString nonClickableCategory;
-    const int colonPosition = text.indexOf(QStringLiteral(": "));
-
-    if (colonPosition != -1) {
-        nonClickableCategory = text.mid(0, colonPosition + 2);
-        clickableText = text.mid(colonPosition + 2);
-    }
-
-    return nonClickableCategory + wrapInLink(clickableText, QLatin1String(LINK_ACTION_APPLY_FIX));
-}
-
-void openEditorAt(const ClangBackEnd::SourceLocationContainer &location)
-{
     Core::EditorManager::openEditorAt(location.filePath().toString(),
                                       int(location.line()),
                                       int(location.column() - 1));
 }
 
-void applyFixit(const QVector<ClangBackEnd::FixItContainer> &fixits)
+void applyFixit(const ClangBackEnd::DiagnosticContainer &diagnostic)
 {
-    ClangCodeModel::ClangFixItOperation operation(Utf8String(), fixits);
+    ClangCodeModel::ClangFixItOperation operation(Utf8String(), diagnostic.fixIts());
 
     operation.perform();
 }
 
-template <typename LayoutType>
+class WidgetFromDiagnostics
-LayoutType *createLayout()
 {
-    auto *layout = new LayoutType;
-    layout->setContentsMargins(0, 0, 0, 0);
-    layout->setSpacing(2);
-
-    return layout;
-}
-
-enum IndentType { IndentDiagnostic, DoNotIndentDiagnostic };
-
-QWidget *createDiagnosticLabel(const ClangBackEnd::DiagnosticContainer &diagnostic,
-                               const QString &mainFilePath,
-                               IndentType indentType = DoNotIndentDiagnostic,
-                               bool enableClickableFixits = true)
-{
-    const bool hasFixit = enableClickableFixits ? !diagnostic.fixIts().isEmpty() : false;
-    const QString diagnosticText = diagnostic.text().toString().toHtmlEscaped();
-    const QString text = clickableLocation(mainFilePath, diagnostic.location())
-                       + QStringLiteral(": ")
-                       + clickableFixIt(diagnosticText, hasFixit);
-    const ClangBackEnd::SourceLocationContainer location = diagnostic.location();
-    const QVector<ClangBackEnd::FixItContainer> fixits = diagnostic.fixIts();
-
-    auto *label = new QLabel(text);
-    if (indentType == IndentDiagnostic)
-        label->setContentsMargins(childIndentationOnTheLeftInPixel, 0, 0, 0);
-    label->setTextFormat(Qt::RichText);
-    QObject::connect(label, &QLabel::linkActivated, [location, fixits](const QString &action) {
-        if (action == QLatin1String(LINK_ACTION_APPLY_FIX))
-            applyFixit(fixits);
-        else
-            openEditorAt(location);
-
-        Utils::ToolTip::hideImmediately();
-    });
-
-    return label;
-}
-
-class MainDiagnosticWidget : public QWidget
-{
-    Q_OBJECT
 public:
-    MainDiagnosticWidget(const ClangBackEnd::DiagnosticContainer &diagnostic,
+    struct DisplayHints {
-                         const ClangCodeModel::Internal::DisplayHints &displayHints)
+        bool showCategoryAndEnableOption;
+        bool showFileNameInMainDiagnostic;
+        bool enableClickableFixits;
+        bool limitWidth;
+        bool hideTooltipAfterLinkActivation;
+    };
+
+    static QWidget *create(const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics,
+                           const DisplayHints &displayHints)
     {
-        setContentsMargins(0, 0, 0, 0);
+        WidgetFromDiagnostics converter(displayHints);
-        auto *mainLayout = createLayout<QVBoxLayout>();
+        return converter.createWidget(diagnostics);
+    }
 
-        const ClangBackEnd::SourceLocationContainer location = diagnostic.location();
+private:
+    enum class IndentMode { Indent, DoNotIndent };
 
-        // Set up header row: category + responsible option
+    WidgetFromDiagnostics(const DisplayHints &displayHints)
-        if (displayHints.showMainDiagnosticHeader) {
+        : m_displayHints(displayHints)
-            const QString category = diagnostic.category();
+    {
-            const QString responsibleOption = diagnostic.enableOption();
+    }
 
-            auto *headerLayout = createLayout<QHBoxLayout>();
+    QWidget *createWidget(const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics)
-            headerLayout->addWidget(new QLabel(wrapInBoldTags(category)), 1);
+    {
+        const QString text = htmlText(diagnostics);
 
-            auto *responsibleOptionLabel = new QLabel(wrapInColor(responsibleOption, "gray"));
+        auto *label = new QLabel;
-            headerLayout->addWidget(responsibleOptionLabel, 0);
+        label->setTextFormat(Qt::RichText);
-            mainLayout->addLayout(headerLayout);
+        label->setText(text);
+        label->setTextInteractionFlags(Qt::TextBrowserInteraction);
+        // Using "setWordWrap(true)" alone will wrap the text already for small
+        // widths, so do not require word wrapping until we hit limits.
+        if (m_displayHints.limitWidth && label->sizeHint().width() > widthLimit()) {
+            label->setMaximumWidth(widthLimit());
+            label->setWordWrap(true);
         }
 
-        // Set up main row: diagnostic text
+        const TargetIdToDiagnosticTable table = m_targetIdsToDiagnostics;
-        const Utf8String mainFilePath = displayHints.showFileNameInMainDiagnostic
+        const bool hideToolTipAfterLinkActivation = m_displayHints.hideTooltipAfterLinkActivation;
+        QObject::connect(label, &QLabel::linkActivated, [table, hideToolTipAfterLinkActivation]
+                         (const QString &action) {
+            const ClangBackEnd::DiagnosticContainer diagnostic = table.value(action);
+            QTC_ASSERT(diagnostic != ClangBackEnd::DiagnosticContainer(), return);
+
+            if (action.startsWith(LINK_ACTION_APPLY_FIX))
+                applyFixit(diagnostic);
+            else
+                openEditorAt(diagnostic);
+
+            if (hideToolTipAfterLinkActivation)
+                Utils::ToolTip::hideImmediately();
+        });
+
+        return label;
+    }
+
+    QString htmlText(const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics)
+    {
+        // For debugging, add: style='border-width:1px;border-color:black'
+        QString text = "<table cellspacing='0' cellpadding='0'>";
+
+        foreach (const ClangBackEnd::DiagnosticContainer &diagnostic, diagnostics)
+            text.append(tableRows(diagnostic));
+
+        text.append("</table>");
+
+        return text;
+    }
+
+    QString tableRows(const ClangBackEnd::DiagnosticContainer &diagnostic)
+    {
+        m_mainFilePath = m_displayHints.showFileNameInMainDiagnostic
                 ? Utf8String()
-                : location.filePath();
+                : diagnostic.location().filePath();
-        mainLayout->addWidget(createDiagnosticLabel(diagnostic,
-                                                    mainFilePath,
-                                                    DoNotIndentDiagnostic,
-                                                    displayHints.enableClickableFixits));
 
-        setLayout(mainLayout);
+        QString text;
+
+        if (m_displayHints.showCategoryAndEnableOption)
+            text.append(diagnosticCategoryAndEnableOptionRow(diagnostic));
+        text.append(diagnosticRow(diagnostic, IndentMode::DoNotIndent));
+        text.append(diagnosticRowsForChildren(diagnostic));
+
+        return text;
     }
+
+    static QString diagnosticCategoryAndEnableOptionRow(
+            const ClangBackEnd::DiagnosticContainer &diagnostic)
+    {
+        const QString text = QString::fromLatin1(
+            "  <tr>"
+            "    <td align='left'><b>%1</b></td>"
+            "    <td align='right'><font color='gray'>%2</font></td>"
+            "  </tr>")
+            .arg(diagnostic.category(), diagnostic.enableOption());
+
+        return text;
+    }
+
+    QString diagnosticText(const ClangBackEnd::DiagnosticContainer &diagnostic)
+    {
+        const bool hasFixit = m_displayHints.enableClickableFixits
+                && !diagnostic.fixIts().isEmpty();
+        const QString diagnosticText = diagnostic.text().toString().toHtmlEscaped();
+
+        // For debugging, add to <table>: style='border-width:1px;border-color:red'
+        const QString text = QString::fromLatin1(
+            "<table cellspacing='0' cellpadding='0'>"
+            "  <tr>"
+            "    <td>%1: </td>"
+            "    <td width='100%'>%2</td>"
+            "  </tr>"
+            "</table>")
+            .arg(clickableLocation(diagnostic, m_mainFilePath),
+                 clickableFixIt(diagnostic, diagnosticText, hasFixit));
+
+        return text;
+    }
+
+    QString diagnosticRow(const ClangBackEnd::DiagnosticContainer &diagnostic,
+                          IndentMode indentMode)
+    {
+        const QString text = QString::fromLatin1(
+            "  <tr>"
+            "    <td colspan='2' align='left' style='%1'>%2</td>"
+            "  </tr>")
+            .arg(indentModeToHtmlStyle(indentMode),
+                 diagnosticText(diagnostic));
+
+        return text;
+    }
+
+    QString diagnosticRowsForChildren(const ClangBackEnd::DiagnosticContainer &diagnostic)
+    {
+        const QVector<ClangBackEnd::DiagnosticContainer> children = diagnostic.children();
+        QString text;
+
+        if (children.size() <= 10) {
+            text += diagnosticRowsForChildren(children.begin(), children.end());
+        } else {
+            text += diagnosticRowsForChildren(children.begin(), children.begin() + 7);
+            text += ellipsisRow();
+            text += diagnosticRowsForChildren(children.end() - 3, children.end());
+        }
+
+        return text;
+    }
+
+    QString diagnosticRowsForChildren(
+            const QVector<ClangBackEnd::DiagnosticContainer>::const_iterator first,
+            const QVector<ClangBackEnd::DiagnosticContainer>::const_iterator last)
+    {
+        QString text;
+
+        for (auto it = first; it != last; ++it)
+            text.append(diagnosticRow(*it, IndentMode::Indent));
+
+        return text;
+    }
+
+    QString clickableLocation(const ClangBackEnd::DiagnosticContainer &diagnostic,
+                              const QString &mainFilePath)
+    {
+        const ClangBackEnd::SourceLocationContainer location = diagnostic.location();
+
+        const QString filePrefix = fileNamePrefix(mainFilePath, location);
+        const QString lineColumn = locationToString(location);
+        const QString linkText = filePrefix + lineColumn;
+        const QString targetId = generateTargetId(LINK_ACTION_GOTO_LOCATION, diagnostic);
+
+        return wrapInLink(linkText, targetId);
+    }
+
+    QString clickableFixIt(const ClangBackEnd::DiagnosticContainer &diagnostic,
+                           const QString &text,
+                           bool hasFixIt)
+    {
+        if (!hasFixIt)
+            return text;
+
+        QString clickableText = text;
+        QString nonClickableCategory;
+        const int colonPosition = text.indexOf(QStringLiteral(": "));
+
+        if (colonPosition != -1) {
+            nonClickableCategory = text.mid(0, colonPosition + 2);
+            clickableText = text.mid(colonPosition + 2);
+        }
+
+        const QString targetId = generateTargetId(LINK_ACTION_APPLY_FIX, diagnostic);
+
+        return nonClickableCategory + wrapInLink(clickableText, targetId);
+    }
+
+    QString generateTargetId(const QString &targetPrefix,
+                             const ClangBackEnd::DiagnosticContainer &diagnostic)
+    {
+        const QString idAsString = QString::number(++m_targetIdCounter);
+        const QString targetId = targetPrefix + idAsString;
+        m_targetIdsToDiagnostics.insert(targetId, diagnostic);
+
+        return targetId;
+    }
+
+    static QString wrapInLink(const QString &text, const QString &target)
+    {
+        return QStringLiteral("<a href='%1' style='text-decoration:none'>%2</a>").arg(target, text);
+    }
+
+    static QString ellipsisRow()
+    {
+        return QString::fromLatin1(
+            "  <tr>"
+            "    <td colspan='2' align='left' style='%1'>...</td>"
+            "  </tr>")
+            .arg(indentModeToHtmlStyle(IndentMode::Indent));
+    }
+
+    static QString indentModeToHtmlStyle(IndentMode indentMode)
+    {
+        return indentMode == IndentMode::Indent
+             ? QString("padding-left:10px")
+             : QString();
+    }
+
+    static int widthLimit()
+    {
+        return QApplication::desktop()->availableGeometry(QCursor::pos()).width() / 2;
+    }
+
+private:
+    const DisplayHints m_displayHints;
+
+    using TargetIdToDiagnosticTable = QHash<QString, ClangBackEnd::DiagnosticContainer>;
+    TargetIdToDiagnosticTable m_targetIdsToDiagnostics;
+    unsigned m_targetIdCounter = 0;
+
+    QString m_mainFilePath;
 };
 
-void addChildrenToLayout(const QString &mainFilePath,
-                         const QVector<ClangBackEnd::DiagnosticContainer>::const_iterator first,
-                         const QVector<ClangBackEnd::DiagnosticContainer>::const_iterator last,
-                         bool enableClickableFixits,
-                         QLayout &boxLayout)
-{
-    for (auto it = first; it != last; ++it) {
-        boxLayout.addWidget(createDiagnosticLabel(*it,
-                                                  mainFilePath,
-                                                  IndentDiagnostic,
-                                                  enableClickableFixits));
-    }
-}
-
-void setupChildDiagnostics(const QString &mainFilePath,
-                           const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics,
-                           bool enableClickableFixits,
-                           QLayout &boxLayout)
-{
-    if (diagnostics.size() <= 10) {
-        addChildrenToLayout(mainFilePath, diagnostics.begin(), diagnostics.end(),
-                            enableClickableFixits, boxLayout);
-    } else {
-        addChildrenToLayout(mainFilePath, diagnostics.begin(), diagnostics.begin() + 7,
-                            enableClickableFixits, boxLayout);
-
-        auto ellipsisLabel = new QLabel(QStringLiteral("..."));
-        ellipsisLabel->setContentsMargins(childIndentationOnTheLeftInPixel, 0, 0, 0);
-        boxLayout.addWidget(ellipsisLabel);
-
-        addChildrenToLayout(mainFilePath, diagnostics.end() - 3, diagnostics.end(),
-                            enableClickableFixits, boxLayout);
-    }
-}
-
 } // anonymous namespace
 
 namespace ClangCodeModel {
 namespace Internal {
 
-void addToolTipToLayout(const ClangBackEnd::DiagnosticContainer &diagnostic,
+QWidget *ClangDiagnosticWidget::create(
-                        QLayout *target,
+        const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics,
-                        const DisplayHints &displayHints)
+        const Destination &destination)
 {
-    // Set up header and text row for main diagnostic
+    WidgetFromDiagnostics::DisplayHints hints;
-    target->addWidget(new MainDiagnosticWidget(diagnostic, displayHints));
 
-    // Set up child rows for notes
+    if (destination == ToolTip) {
-    setupChildDiagnostics(diagnostic.location().filePath(),
+        hints.showCategoryAndEnableOption = true;
-                          diagnostic.children(),
+        hints.showFileNameInMainDiagnostic = false;
-                          displayHints.enableClickableFixits,
+        hints.enableClickableFixits = true;
-                          *target);
+        hints.limitWidth = true;
+        hints.hideTooltipAfterLinkActivation = true;
+    } else { // Info Bar
+        hints.showCategoryAndEnableOption = false;
+        hints.showFileNameInMainDiagnostic = true;
+        // Clickable fixits might change toolchain headers, so disable.
+        hints.enableClickableFixits = false;
+        hints.limitWidth = false;
+        hints.hideTooltipAfterLinkActivation = false;
+    }
+
+    return WidgetFromDiagnostics::create(diagnostics, hints);
 }
 
 } // namespace Internal
 } // namespace ClangCodeModel
-
-#include "clangdiagnostictooltipwidget.moc"

src/plugins/clangcodemodel/clangdiagnostictooltipwidget.h

@@ -34,15 +34,13 @@ QT_END_NAMESPACE
 namespace ClangCodeModel {
 namespace Internal {
 
-struct DisplayHints {
+class ClangDiagnosticWidget {
-    bool showMainDiagnosticHeader = true;
+public:
-    bool showFileNameInMainDiagnostic = false;
+    enum Destination { ToolTip, InfoBar };
-    bool enableClickableFixits = true;
-};
 
-void addToolTipToLayout(const ClangBackEnd::DiagnosticContainer &diagnostic,
+    static QWidget *create(const QVector<ClangBackEnd::DiagnosticContainer> &diagnostics,
-                        QLayout *target,
+                           const Destination &destination);
-                        const DisplayHints &displayHints = DisplayHints());
+};
 
 } // namespace Internal
 } // namespace ClangCodeModel

src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp

@@ -263,8 +263,12 @@ void ClangEditorDocumentProcessor::addDiagnosticToolTipToLayout(uint line,
                                                                 uint column,
                                                                 QLayout *target) const
 {
-    foreach (const auto &diagnostic, m_diagnosticManager.diagnosticsAt(line, column))
+    using Internal::ClangDiagnosticWidget;
-        addToolTipToLayout(diagnostic, target);
+
+    const QVector<ClangBackEnd::DiagnosticContainer> diagnostics
+        = m_diagnosticManager.diagnosticsAt(line, column);
+
+    target->addWidget(ClangDiagnosticWidget::create(diagnostics, ClangDiagnosticWidget::ToolTip));
 }
 
 void ClangEditorDocumentProcessor::editorDocumentTimerRestarted()
@@ -342,16 +346,6 @@ void ClangEditorDocumentProcessor::requestDocumentAnnotations(const QString &pro
     m_ipcCommunicator.requestDocumentAnnotations(fileContainer);
 }
 
-static Internal::DisplayHints displayHintsForInfoBar()
-{
-    Internal::DisplayHints displayHints;
-    displayHints.showMainDiagnosticHeader = false;
-    displayHints.showFileNameInMainDiagnostic = true;
-    displayHints.enableClickableFixits = false; // Tool chain headers might be changed, so disable.
-
-    return displayHints;
-}
-
 CppTools::BaseEditorDocumentProcessor::HeaderErrorDiagnosticWidgetCreator
 ClangEditorDocumentProcessor::creatorForHeaderErrorDiagnosticWidget(
         const ClangBackEnd::DiagnosticContainer &firstHeaderErrorDiagnostic)
@@ -365,7 +359,8 @@ ClangEditorDocumentProcessor::creatorForHeaderErrorDiagnosticWidget(
         vbox->setContentsMargins(10, 0, 0, 2);
         vbox->setSpacing(2);
 
-        addToolTipToLayout(firstHeaderErrorDiagnostic, vbox, displayHintsForInfoBar());
+        vbox->addWidget(ClangDiagnosticWidget::create({firstHeaderErrorDiagnostic},
+                                                      ClangDiagnosticWidget::InfoBar));
 
         auto widget = new QWidget;
         widget->setLayout(vbox);

src/plugins/clangcodemodel/clangtextmark.cpp

@@ -31,6 +31,7 @@
 #include <utils/icon.h>
 #include <utils/theme/theme.h>
 
+#include <QLayout>
 #include <QString>
 
 namespace ClangCodeModel {
@@ -84,7 +85,11 @@ void ClangTextMark::setIcon(ClangBackEnd::DiagnosticSeverity severity)
 
 bool ClangTextMark::addToolTipContent(QLayout *target)
 {
-    Internal::addToolTipToLayout(m_diagnostic, target, Internal::DisplayHints());
+    using Internal::ClangDiagnosticWidget;
+
+    QWidget *widget = ClangDiagnosticWidget::create({m_diagnostic}, ClangDiagnosticWidget::ToolTip);
+    target->addWidget(widget);
+
     return true;
 }