Clang: Change the way completion fix-its are shown

Move the completion information to the tooltip and
show fix-it icon on the right of such item.

Change-Id: I7eff410384104387e547695171e4864760c07fb9
Reviewed-by: Nikolai Kosjar <nikolai.kosjar@qt.io>

src/libs/utils/textutils.cpp

@@ -39,8 +39,9 @@ bool convertPosition(const QTextDocument *document, int pos, int *line, int *col
         (*column) = -1;
         return false;
     } else {
+        // line and column are both 1-based
         (*line) = block.blockNumber() + 1;
-        (*column) = pos - block.position();
+        (*column) = pos - block.position() + 1;
         return true;
     }
 }

src/plugins/clangcodemodel/clangassistproposalitem.cpp

@@ -27,6 +27,7 @@
 
 #include "clangcompletionchunkstotextconverter.h"
 #include "clangfixitoperation.h"
+#include "clangutils.h"
 
 #include <cplusplus/Icons.h>
 #include <cplusplus/MatchingText.h>
@@ -34,10 +35,13 @@
 #include <cplusplus/Token.h>
 
 #include <texteditor/completionsettings.h>
+#include <texteditor/texteditor.h>
 #include <texteditor/texteditorsettings.h>
 
 #include <QCoreApplication>
+#include <QTextBlock>
 #include <QTextCursor>
+#include <QTextDocument>
 
 #include <utils/algorithm.h>
 #include <utils/textutils.h>
@@ -45,6 +49,7 @@
 
 using namespace CPlusPlus;
 using namespace ClangBackEnd;
+using namespace TextEditor;
 
 namespace ClangCodeModel {
 namespace Internal {
@@ -73,7 +78,7 @@ bool ClangAssistProposalItem::implicitlyApplies() const
     return true;
 }
 
-static QString textUntilPreviousStatement(TextEditor::TextDocumentManipulatorInterface &manipulator,
+static QString textUntilPreviousStatement(TextDocumentManipulatorInterface &manipulator,
                                           int startPosition)
 {
     static const QString stopCharacters(";{}#");
@@ -90,7 +95,7 @@ static QString textUntilPreviousStatement(TextEditor::TextDocumentManipulatorInt
 }
 
 // 7.3.3: using typename(opt) nested-name-specifier unqualified-id ;
-static bool isAtUsingDeclaration(TextEditor::TextDocumentManipulatorInterface &manipulator,
+static bool isAtUsingDeclaration(TextDocumentManipulatorInterface &manipulator,
                                  int basePosition)
 {
     SimpleLexer lexer;
@@ -105,7 +110,7 @@ static bool isAtUsingDeclaration(TextEditor::TextDocumentManipulatorInterface &m
     if (lastToken.kind() != T_COLON_COLON)
         return false;
 
-    return Utils::contains(tokens, [](const Token &token) {
+    return ::Utils::contains(tokens, [](const Token &token) {
         return token.kind() == T_USING;
     });
 }
@@ -135,17 +140,17 @@ static QString methodDefinitionParameters(const CodeCompletionChunks &chunks)
     return result;
 }
 
-void ClangAssistProposalItem::apply(TextEditor::TextDocumentManipulatorInterface &manipulator,
+void ClangAssistProposalItem::apply(TextDocumentManipulatorInterface &manipulator,
                                     int basePosition) const
 {
     const CodeCompletion ccr = firstCodeCompletion();
 
     if (!ccr.requiredFixIts.empty()) {
+        // Important: Calculate shift before changing the document.
+        basePosition += fixItsShift(manipulator);
+
         ClangFixItOperation fixItOperation(Utf8String(), ccr.requiredFixIts);
         fixItOperation.perform();
-
-        const int shift = fixItsShift(manipulator);
-        basePosition += shift;
     }
 
     QString textToBeInserted = m_text;
@@ -175,8 +180,8 @@ void ClangAssistProposalItem::apply(TextEditor::TextDocumentManipulatorInterface
 
         textToBeInserted = converter.text();
     } else if (!ccr.text.isEmpty()) {
-        const TextEditor::CompletionSettings &completionSettings =
+        const CompletionSettings &completionSettings =
-                TextEditor::TextEditorSettings::instance()->completionSettings();
+                TextEditorSettings::instance()->completionSettings();
         const bool autoInsertBrackets = completionSettings.m_autoInsertBrackets;
 
         if (autoInsertBrackets &&
@@ -193,9 +198,9 @@ void ClangAssistProposalItem::apply(TextEditor::TextDocumentManipulatorInterface
             QTextCursor cursor = manipulator.textCursorAt(basePosition);
 
             bool abandonParen = false;
-            if (Utils::Text::matchPreviousWord(manipulator, cursor, "&")) {
+            if (::Utils::Text::matchPreviousWord(manipulator, cursor, "&")) {
-                Utils::Text::moveToPrevChar(manipulator, cursor);
+                ::Utils::Text::moveToPrevChar(manipulator, cursor);
-                Utils::Text::moveToPrevChar(manipulator, cursor);
+                ::Utils::Text::moveToPrevChar(manipulator, cursor);
                 const QChar prevChar = manipulator.characterAt(cursor.position());
                 cursor.setPosition(basePosition);
                 abandonParen = QString("(;,{}").contains(prevChar);
@@ -206,7 +211,7 @@ void ClangAssistProposalItem::apply(TextEditor::TextDocumentManipulatorInterface
             if (!abandonParen && ccr.completionKind == CodeCompletion::FunctionDefinitionCompletionKind) {
                 const CodeCompletionChunk resultType = ccr.chunks.first();
                 QTC_ASSERT(resultType.kind == CodeCompletionChunk::ResultType, return;);
-                if (Utils::Text::matchPreviousWord(manipulator, cursor, resultType.text.toString())) {
+                if (::Utils::Text::matchPreviousWord(manipulator, cursor, resultType.text.toString())) {
                     extraCharacters += methodDefinitionParameters(ccr.chunks);
                     // To skip the next block.
                     abandonParen = true;
@@ -306,7 +311,7 @@ void ClangAssistProposalItem::setText(const QString &text)
 
 QString ClangAssistProposalItem::text() const
 {
-    return m_text + (requiresFixIts() ? fixItText() : QString());
+    return m_text;
 }
 
 const QVector<ClangBackEnd::FixItContainer> &ClangAssistProposalItem::firstCompletionFixIts() const
@@ -314,39 +319,53 @@ const QVector<ClangBackEnd::FixItContainer> &ClangAssistProposalItem::firstCompl
     return firstCodeCompletion().requiredFixIts;
 }
 
-// FIXME: Indicate required fix-it without adding extra text.
+std::pair<int, int> fixItPositionsRange(const FixItContainer &fixIt, const QTextCursor &cursor)
+{
+    const QTextBlock startLine = cursor.document()->findBlockByNumber(fixIt.range.start.line - 1);
+    const QTextBlock endLine = cursor.document()->findBlockByNumber(fixIt.range.end.line - 1);
+
+    const int fixItStartPos = ::Utils::Text::positionInText(
+                cursor.document(),
+                static_cast<int>(fixIt.range.start.line),
+                Utils::cppEditorColumn(startLine, static_cast<int>(fixIt.range.start.column)));
+    const int fixItEndPos = ::Utils::Text::positionInText(
+                cursor.document(),
+                static_cast<int>(fixIt.range.end.line),
+                Utils::cppEditorColumn(endLine, static_cast<int>(fixIt.range.end.column)));
+    return std::make_pair(fixItStartPos, fixItEndPos);
+}
+
+static QString textReplacedByFixit(const FixItContainer &fixIt)
+{
+    TextEditorWidget *textEditorWidget = TextEditorWidget::currentTextEditorWidget();
+    if (!textEditorWidget)
+        return QString();
+    const std::pair<int, int> fixItPosRange = fixItPositionsRange(fixIt,
+                                                                  textEditorWidget->textCursor());
+    return textEditorWidget->textAt(fixItPosRange.first,
+                                    fixItPosRange.second - fixItPosRange.first);
+}
+
 QString ClangAssistProposalItem::fixItText() const
 {
     const FixItContainer &fixIt = firstCompletionFixIts().first();
-    const SourceRangeContainer &range = fixIt.range;
     return QCoreApplication::translate("ClangCodeModel::ClangAssistProposalItem",
-                                       " (requires to correct [%1:%2-%3:%4] to \"%5\")")
+                                       "Requires to correct \"%1\" to \"%2\"")
-            .arg(range.start.line)
+            .arg(textReplacedByFixit(fixIt))
-            .arg(range.start.column)
-            .arg(range.end.line)
-            .arg(range.end.column)
             .arg(fixIt.text.toString());
 }
 
-int ClangAssistProposalItem::fixItsShift(
+int ClangAssistProposalItem::fixItsShift(const TextDocumentManipulatorInterface &manipulator) const
-        const TextEditor::TextDocumentManipulatorInterface &manipulator) const
 {
     const QVector<ClangBackEnd::FixItContainer> &requiredFixIts = firstCompletionFixIts();
     if (requiredFixIts.empty())
         return 0;
 
     int shift = 0;
-    QTextCursor cursor = manipulator.textCursorAt(0);
+    const QTextCursor cursor = manipulator.textCursorAt(0);
     for (const FixItContainer &fixIt : requiredFixIts) {
-        const int fixItStartPos = Utils::Text::positionInText(
+        const std::pair<int, int> fixItPosRange = fixItPositionsRange(fixIt, cursor);
-                    cursor.document(),
+        shift += fixIt.text.toString().length() - (fixItPosRange.second - fixItPosRange.first);
-                    static_cast<int>(fixIt.range.start.line),
-                    static_cast<int>(fixIt.range.start.column));
-        const int fixItEndPos = Utils::Text::positionInText(
-                    cursor.document(),
-                    static_cast<int>(fixIt.range.end.line),
-                    static_cast<int>(fixIt.range.end.column));
-        shift += fixIt.text.toString().length() - (fixItEndPos - fixItStartPos);
     }
     return shift;
 }
@@ -425,10 +444,14 @@ QString ClangAssistProposalItem::detail() const
             detail += "<br>";
         detail += CompletionChunksToTextConverter::convertToToolTipWithHtml(
                     codeCompletion.chunks, codeCompletion.completionKind);
+
         if (!codeCompletion.briefComment.isEmpty())
             detail += "<br>" + codeCompletion.briefComment.toString();
     }
 
+    if (requiresFixIts())
+        detail += "<br><br><b>" + fixItText() + "</b>";
+
     return detail;
 }
 

src/plugins/clangcodemodel/clangutils.cpp

@@ -155,8 +155,18 @@ int clangColumn(const QTextBlock &line, int cppEditorColumn)
     // (2) The return value is the column in Clang which is the utf8 byte offset from the beginning
     //     of the line.
     // Here we convert column from (1) to (2).
-    // '+ 1' is for 1-based columns
+    // '- 1' and '+ 1' are because of 1-based columns
-    return line.text().left(cppEditorColumn).toUtf8().size() + 1;
+    return line.text().left(cppEditorColumn - 1).toUtf8().size() + 1;
+}
+
+int cppEditorColumn(const QTextBlock &line, int clangColumn)
+{
+    // (1) clangColumn is the column in Clang which is the utf8 byte offset from the beginning
+    //     of the line.
+    // (2) The return value is the actual column shown by CppEditor.
+    // Here we convert column from (1) to (2).
+    // '- 1' and '+ 1' are because of 1-based columns
+    return QString::fromUtf8(line.text().toUtf8().left(clangColumn - 1)).size() + 1;
 }
 
 ::Utils::CodeModelIcon::Type iconTypeForToken(const ClangBackEnd::TokenInfoContainer &token)

src/plugins/clangcodemodel/clangutils.h

@@ -57,7 +57,8 @@ CppTools::ProjectPart::Ptr projectPartForFile(const QString &filePath);
 CppTools::ProjectPart::Ptr projectPartForFileBasedOnProcessor(const QString &filePath);
 bool isProjectPartLoaded(const CppTools::ProjectPart::Ptr projectPart);
 QString projectPartIdForFile(const QString &filePath);
-int clangColumn(const QTextBlock &lineText, int cppEditorColumn);
+int clangColumn(const QTextBlock &line, int cppEditorColumn);
+int cppEditorColumn(const QTextBlock &line, int clangColumn);
 
 QString diagnosticCategoryPrefixRemoved(const QString &text);
 

src/plugins/texteditor/codeassist/genericproposalwidget.cpp

@@ -31,9 +31,11 @@
 #include <texteditor/texteditorsettings.h>
 #include <texteditor/completionsettings.h>
 #include <texteditor/texteditorconstants.h>
+#include <texteditor/codeassist/assistproposaliteminterface.h>
 
 #include <utils/faketooltip.h>
 #include <utils/hostosinfo.h>
+#include <utils/utilsicons.h>
 
 #include <QRect>
 #include <QLatin1String>
@@ -49,6 +51,7 @@
 #include <QKeyEvent>
 #include <QDesktopWidget>
 #include <QLabel>
+#include <QStyledItemDelegate>
 
 using namespace Utils;
 
@@ -92,6 +95,8 @@ QVariant ModelAdapter::data(const QModelIndex &index, int role) const
         return m_completionModel->icon(index.row());
     else if (role == Qt::WhatsThisRole)
         return m_completionModel->detail(index.row());
+    else if (role == Qt::UserRole)
+        return m_completionModel->proposalItem(index.row())->requiresFixIts();
 
     return QVariant();
 }
@@ -146,6 +151,7 @@ private:
 // -----------------------
 class GenericProposalListView : public QListView
 {
+    friend class ProposalItemDelegate;
 public:
     GenericProposalListView(QWidget *parent);
 
@@ -160,10 +166,53 @@ public:
     void selectLastRow() { selectRow(model()->rowCount() - 1); }
 };
 
+class ProposalItemDelegate : public QStyledItemDelegate
+{
+    Q_OBJECT
+public:
+    explicit ProposalItemDelegate(GenericProposalListView *parent = nullptr)
+        : QStyledItemDelegate(parent)
+        , m_parent(parent)
+    {
+    }
+
+    void paint(QPainter *painter,
+               const QStyleOptionViewItem &option,
+               const QModelIndex &index) const override
+    {
+        static const QIcon fixItIcon = ::Utils::Icons::CODEMODEL_FIXIT.icon();
+
+        QStyledItemDelegate::paint(painter, option, index);
+
+        if (m_parent->model()->data(index, Qt::UserRole).toBool()) {
+            const QRect itemRect = m_parent->rectForIndex(index);
+            const QScrollBar *verticalScrollBar = m_parent->verticalScrollBar();
+
+            const int x = m_parent->width() - itemRect.height() - (verticalScrollBar->isVisible()
+                                                                   ? verticalScrollBar->width()
+                                                                   : 0);
+            const int iconSize = itemRect.height() - 5;
+            fixItIcon.paint(painter, QRect(x, itemRect.y() - m_parent->verticalOffset(),
+                                           iconSize, iconSize));
+        }
+    }
+
+    QSize sizeHint(const QStyleOptionViewItem &option, const QModelIndex &index) const override
+    {
+        QSize size(QStyledItemDelegate::sizeHint(option, index));
+        if (m_parent->model()->data(index, Qt::UserRole).toBool())
+            size.setWidth(size.width() + m_parent->rectForIndex(index).height() - 5);
+        return size;
+    }
+private:
+    GenericProposalListView *m_parent;
+};
+
 GenericProposalListView::GenericProposalListView(QWidget *parent)
     : QListView(parent)
 {
     setVerticalScrollMode(QAbstractItemView::ScrollPerItem);
+    setItemDelegate(new ProposalItemDelegate(this));
 }
 
 QSize GenericProposalListView::calculateSize() const