From 07c5cf3a83e8e69497f5146b9480499cdfc687a2 Mon Sep 17 00:00:00 2001 From: Nikolai Kosjar Date: Tue, 18 Oct 2016 18:01:57 +0200 Subject: [PATCH] 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 Reviewed-by: David Schulz --- .../clangbackendipc/diagnosticcontainer.h | 5 + .../clangdiagnostictooltipwidget.cpp | 420 +++++++++++------- .../clangdiagnostictooltipwidget.h | 14 +- .../clangeditordocumentprocessor.cpp | 21 +- src/plugins/clangcodemodel/clangtextmark.cpp | 7 +- 5 files changed, 285 insertions(+), 182 deletions(-) diff --git a/src/libs/clangbackendipc/diagnosticcontainer.h b/src/libs/clangbackendipc/diagnosticcontainer.h index b9d47db04e1..677fb813d5a 100644 --- a/src/libs/clangbackendipc/diagnosticcontainer.h +++ b/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 ranges_; diff --git a/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.cpp b/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.cpp index 5b6c48c9536..1275ab81cd3 100644 --- a/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.cpp +++ b/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.cpp @@ -28,34 +28,22 @@ #include +#include #include +#include +#include #include -#include +#include #include -#include -#include + +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("") + text + QStringLiteral(""); -} - -QString wrapInLink(const QString &text, const QString &target) -{ - return QStringLiteral("%2").arg(target, text); -} - -QString wrapInColor(const QString &text, const QByteArray &color) -{ - return QStringLiteral("%1").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, - const ClangBackEnd::SourceLocationContainer &location) +void openEditorAt(const ClangBackEnd::DiagnosticContainer &diagnostic) { - const QString filePrefix = fileNamePrefix(mainFilePath, location); - const QString lineColumn = locationToString(location); - const QString linkText = filePrefix + lineColumn; + const ClangBackEnd::SourceLocationContainer location = diagnostic.location(); - 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 &fixits) +void applyFixit(const ClangBackEnd::DiagnosticContainer &diagnostic) { - ClangCodeModel::ClangFixItOperation operation(Utf8String(), fixits); + ClangCodeModel::ClangFixItOperation operation(Utf8String(), diagnostic.fixIts()); operation.perform(); } -template -LayoutType *createLayout() +class WidgetFromDiagnostics { - 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 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, - const ClangCodeModel::Internal::DisplayHints &displayHints) + struct DisplayHints { + bool showCategoryAndEnableOption; + bool showFileNameInMainDiagnostic; + bool enableClickableFixits; + bool limitWidth; + bool hideTooltipAfterLinkActivation; + }; + + static QWidget *create(const QVector &diagnostics, + const DisplayHints &displayHints) { - setContentsMargins(0, 0, 0, 0); - auto *mainLayout = createLayout(); + WidgetFromDiagnostics converter(displayHints); + return converter.createWidget(diagnostics); + } - const ClangBackEnd::SourceLocationContainer location = diagnostic.location(); +private: + enum class IndentMode { Indent, DoNotIndent }; - // Set up header row: category + responsible option - if (displayHints.showMainDiagnosticHeader) { - const QString category = diagnostic.category(); - const QString responsibleOption = diagnostic.enableOption(); + WidgetFromDiagnostics(const DisplayHints &displayHints) + : m_displayHints(displayHints) + { + } - auto *headerLayout = createLayout(); - headerLayout->addWidget(new QLabel(wrapInBoldTags(category)), 1); + QWidget *createWidget(const QVector &diagnostics) + { + const QString text = htmlText(diagnostics); - auto *responsibleOptionLabel = new QLabel(wrapInColor(responsibleOption, "gray")); - headerLayout->addWidget(responsibleOptionLabel, 0); - mainLayout->addLayout(headerLayout); + auto *label = new QLabel; + label->setTextFormat(Qt::RichText); + 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 Utf8String mainFilePath = displayHints.showFileNameInMainDiagnostic + const TargetIdToDiagnosticTable table = m_targetIdsToDiagnostics; + 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 &diagnostics) + { + // For debugging, add: style='border-width:1px;border-color:black' + QString text = ""; + + foreach (const ClangBackEnd::DiagnosticContainer &diagnostic, diagnostics) + text.append(tableRows(diagnostic)); + + text.append("
"); + + return text; + } + + QString tableRows(const ClangBackEnd::DiagnosticContainer &diagnostic) + { + m_mainFilePath = m_displayHints.showFileNameInMainDiagnostic ? Utf8String() - : location.filePath(); - mainLayout->addWidget(createDiagnosticLabel(diagnostic, - mainFilePath, - DoNotIndentDiagnostic, - displayHints.enableClickableFixits)); + : diagnostic.location().filePath(); - 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( + " " + " %1" + " %2" + " ") + .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 : style='border-width:1px;border-color:red' + const QString text = QString::fromLatin1( + "
" + " " + " " + " " + " " + "
%1: %2
") + .arg(clickableLocation(diagnostic, m_mainFilePath), + clickableFixIt(diagnostic, diagnosticText, hasFixit)); + + return text; + } + + QString diagnosticRow(const ClangBackEnd::DiagnosticContainer &diagnostic, + IndentMode indentMode) + { + const QString text = QString::fromLatin1( + " " + " %2" + " ") + .arg(indentModeToHtmlStyle(indentMode), + diagnosticText(diagnostic)); + + return text; + } + + QString diagnosticRowsForChildren(const ClangBackEnd::DiagnosticContainer &diagnostic) + { + const QVector 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::const_iterator first, + const QVector::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("%2").arg(target, text); + } + + static QString ellipsisRow() + { + return QString::fromLatin1( + " " + " ..." + " ") + .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; + TargetIdToDiagnosticTable m_targetIdsToDiagnostics; + unsigned m_targetIdCounter = 0; + + QString m_mainFilePath; }; -void addChildrenToLayout(const QString &mainFilePath, - const QVector::const_iterator first, - const QVector::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 &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, - QLayout *target, - const DisplayHints &displayHints) +QWidget *ClangDiagnosticWidget::create( + const QVector &diagnostics, + const Destination &destination) { - // Set up header and text row for main diagnostic - target->addWidget(new MainDiagnosticWidget(diagnostic, displayHints)); + WidgetFromDiagnostics::DisplayHints hints; - // Set up child rows for notes - setupChildDiagnostics(diagnostic.location().filePath(), - diagnostic.children(), - displayHints.enableClickableFixits, - *target); + if (destination == ToolTip) { + hints.showCategoryAndEnableOption = true; + hints.showFileNameInMainDiagnostic = false; + hints.enableClickableFixits = true; + 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" diff --git a/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.h b/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.h index 839952fc774..3ea1bfb0ada 100644 --- a/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.h +++ b/src/plugins/clangcodemodel/clangdiagnostictooltipwidget.h @@ -34,15 +34,13 @@ QT_END_NAMESPACE namespace ClangCodeModel { namespace Internal { -struct DisplayHints { - bool showMainDiagnosticHeader = true; - bool showFileNameInMainDiagnostic = false; - bool enableClickableFixits = true; -}; +class ClangDiagnosticWidget { +public: + enum Destination { ToolTip, InfoBar }; -void addToolTipToLayout(const ClangBackEnd::DiagnosticContainer &diagnostic, - QLayout *target, - const DisplayHints &displayHints = DisplayHints()); + static QWidget *create(const QVector &diagnostics, + const Destination &destination); +}; } // namespace Internal } // namespace ClangCodeModel diff --git a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp b/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp index 09a7b76b6f1..82f06b59949 100644 --- a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp +++ b/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)) - addToolTipToLayout(diagnostic, target); + using Internal::ClangDiagnosticWidget; + + const QVector 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); diff --git a/src/plugins/clangcodemodel/clangtextmark.cpp b/src/plugins/clangcodemodel/clangtextmark.cpp index 8057d66cb61..fede73fde3f 100644 --- a/src/plugins/clangcodemodel/clangtextmark.cpp +++ b/src/plugins/clangcodemodel/clangtextmark.cpp @@ -31,6 +31,7 @@ #include #include +#include #include 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; }