From 5d19894f5be575d93a2271284dbe74a9dcbf4f41 Mon Sep 17 00:00:00 2001
From: Jarek Kobus <jaroslaw.kobus@qt.io>
Date: Thu, 9 Feb 2023 18:07:41 +0100
Subject: [PATCH] LocatorFilterEntry: Add linkForEditor

Make it of std::optional<Utils::Link> type.

Use it for:
1. Removing duplicates
2. Showing link inside editor

Before, QVariant internalData was used in above cases.

Rationale:
1. Removing duplicates between results from different
   ILocatorFilter makes only sense if accept() overload
   for these filter types are the same (i.e. the implementation
   is repeated). Otherwise, we are loosing some functionality
   when some result is removed. Taking into account that
   original intention was to eliminate duplicates for filters
   where we do BaseFileFilter::openEditorAt() inside accept(),
   we store linkForEditor in 3 cases (instead of storing
   internalData):
 - BaseFileFilter
 - FileSystemFilter (only existing file case)
 - OpenDocumentsFilter

   We don't save a linkForEditor where we stored QString
   inside internalData in the following cases:
 - JavaScriptFilter
 - FileSystemFilter (non existing file case)

   Before, in above cases, we could have filtered out
   some results unintentionally. This patch should fix it.

   Now, we remove duplicates only for enties that have value
   for linkForEditor.

2. Use directly linkForEditor, if exists, for opening editor.

This patch aims to decrease the number of different
responsibilities of internalData by 1.

Make 3rd arg of LocatorFilterEntry c'tor default.

Change-Id: I144c58661d7456bb5991f8077dc103272dfab194
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: David Schulz <david.schulz@qt.io>
---
 .../coreplugin/locator/basefilefilter.cpp     | 27 ++++--------
 .../coreplugin/locator/basefilefilter.h       |  2 +-
 .../coreplugin/locator/executefilter.cpp      |  4 +-
 .../coreplugin/locator/filesystemfilter.cpp   | 21 ++++------
 .../coreplugin/locator/ilocatorfilter.h       |  6 ++-
 .../coreplugin/locator/javascriptfilter.cpp   |  2 +-
 .../coreplugin/locator/locatorsearchutils.cpp | 41 +++++++++----------
 .../coreplugin/locator/locatorsearchutils.h   |  4 +-
 .../locator/opendocumentsfilter.cpp           |  7 ++--
 .../coreplugin/locator/urllocatorfilter.cpp   |  2 +-
 src/plugins/help/helpindexfilter.cpp          |  2 +-
 src/plugins/macros/macrolocatorfilter.cpp     |  2 +-
 .../projectexplorer/projectexplorer.cpp       |  6 +--
 13 files changed, 57 insertions(+), 69 deletions(-)

diff --git a/src/plugins/coreplugin/locator/basefilefilter.cpp b/src/plugins/coreplugin/locator/basefilefilter.cpp
index 260b31d2af2..9ce27cc0528 100644
--- a/src/plugins/coreplugin/locator/basefilefilter.cpp
+++ b/src/plugins/coreplugin/locator/basefilefilter.cpp
@@ -117,8 +117,7 @@ QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFil
     QList<LocatorFilterEntry> entries[int(MatchLevel::Count)];
     // If search string contains spaces, treat them as wildcard '*' and search in full path
     const QString entry = QDir::fromNativeSeparators(origEntry).replace(' ', '*');
-    QString postfix;
-    Link link = Link::fromString(entry, true, &postfix);
+    const Link link = Link::fromString(entry, true);
 
     const QRegularExpression regexp = createRegExp(link.targetFilePath.toString());
     if (!regexp.isValid()) {
@@ -156,10 +155,10 @@ QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFil
         QRegularExpressionMatch match = regexp.match(matchText);
 
         if (match.hasMatch()) {
-            LocatorFilterEntry filterEntry(this, path.fileName(), QString(path.toString() + postfix));
+            LocatorFilterEntry filterEntry(this, path.fileName());
             filterEntry.filePath = path;
             filterEntry.extraInfo = path.shortNativePath();
-
+            filterEntry.linkForEditor = Link(path, link.targetLine, link.targetColumn);
             const MatchLevel matchLevel = matchLevelFor(match, matchText);
             if (hasPathSeparator) {
                 match = regexp.match(filterEntry.extraInfo);
@@ -187,7 +186,7 @@ QList<LocatorFilterEntry> BaseFileFilter::matchesFor(QFutureInterface<LocatorFil
 
     for (auto &entry : entries) {
         if (entry.size() < 1000)
-            Utils::sort(entry, Core::LocatorFilterEntry::compareLexigraphically);
+            Utils::sort(entry, LocatorFilterEntry::compareLexigraphically);
     }
 
     return std::accumulate(std::begin(entries), std::end(entries), QList<LocatorFilterEntry>());
@@ -205,21 +204,13 @@ void BaseFileFilter::accept(const LocatorFilterEntry &selection,
     openEditorAt(selection);
 }
 
-void BaseFileFilter::openEditorAt(const LocatorFilterEntry& selection)
+void BaseFileFilter::openEditorAt(const LocatorFilterEntry &entry)
 {
-    const FilePath locatorText = FilePath::fromVariant(selection.internalData);
-    const int postfixLength = locatorText.fileName().length() - selection.filePath.fileName().length();
-    if (postfixLength > 0) {
-        const QString postfix = selection.internalData.toString().right(postfixLength);
-        int postfixPos = -1;
-        const LineColumn lineColumn = LineColumn::extractFromFileName(postfix, postfixPos);
-        if (postfixPos >= 0) {
-            const Link link(selection.filePath, lineColumn.line, lineColumn.column);
-            EditorManager::openEditorAt(link, {}, Core::EditorManager::AllowExternalEditor);
-            return;
-        }
+    if (entry.linkForEditor) {
+        EditorManager::openEditorAt(*entry.linkForEditor, {}, EditorManager::AllowExternalEditor);
+        return;
     }
-    EditorManager::openEditor(selection.filePath, {}, Core::EditorManager::AllowExternalEditor);
+    EditorManager::openEditor(entry.filePath, {}, EditorManager::AllowExternalEditor);
 }
 
 /*!
diff --git a/src/plugins/coreplugin/locator/basefilefilter.h b/src/plugins/coreplugin/locator/basefilefilter.h
index f1b97a60315..710cd21edf4 100644
--- a/src/plugins/coreplugin/locator/basefilefilter.h
+++ b/src/plugins/coreplugin/locator/basefilefilter.h
@@ -48,7 +48,7 @@ public:
                                          const QString &entry) override;
     void accept(const LocatorFilterEntry &selection,
                 QString *newText, int *selectionStart, int *selectionLength) const override;
-    static void openEditorAt(const LocatorFilterEntry &selection);
+    static void openEditorAt(const LocatorFilterEntry &entry);
 
 protected:
     void setFileIterator(Iterator *iterator);
diff --git a/src/plugins/coreplugin/locator/executefilter.cpp b/src/plugins/coreplugin/locator/executefilter.cpp
index 2983b85d650..6be23fff3f5 100644
--- a/src/plugins/coreplugin/locator/executefilter.cpp
+++ b/src/plugins/coreplugin/locator/executefilter.cpp
@@ -44,7 +44,7 @@ QList<LocatorFilterEntry> ExecuteFilter::matchesFor(QFutureInterface<LocatorFilt
 {
     QList<LocatorFilterEntry> value;
     if (!entry.isEmpty()) // avoid empty entry
-        value.append(LocatorFilterEntry(this, entry, QVariant()));
+        value.append(LocatorFilterEntry(this, entry));
     QList<LocatorFilterEntry> others;
     const Qt::CaseSensitivity entryCaseSensitivity = caseSensitivity(entry);
     for (const QString &cmd : std::as_const(m_commandHistory)) {
@@ -52,7 +52,7 @@ QList<LocatorFilterEntry> ExecuteFilter::matchesFor(QFutureInterface<LocatorFilt
             break;
         if (cmd == entry) // avoid repeated entry
             continue;
-        LocatorFilterEntry filterEntry(this, cmd, QVariant());
+        LocatorFilterEntry filterEntry(this, cmd);
         const int index = cmd.indexOf(entry, 0, entryCaseSensitivity);
         if (index >= 0) {
             filterEntry.highlightInfo = {index, int(entry.length())};
diff --git a/src/plugins/coreplugin/locator/filesystemfilter.cpp b/src/plugins/coreplugin/locator/filesystemfilter.cpp
index 9e085dec79c..17c54c748dc 100644
--- a/src/plugins/coreplugin/locator/filesystemfilter.cpp
+++ b/src/plugins/coreplugin/locator/filesystemfilter.cpp
@@ -109,7 +109,7 @@ QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<LocatorF
         if (match.hasMatch()) {
             const MatchLevel level = matchLevelFor(match, dir);
             const QString fullPath = dirInfo.filePath(dir);
-            LocatorFilterEntry filterEntry(this, dir, QVariant());
+            LocatorFilterEntry filterEntry(this, dir);
             filterEntry.filePath = FilePath::fromString(fullPath);
             filterEntry.highlightInfo = highlightInfo(match);
 
@@ -117,8 +117,7 @@ QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<LocatorF
         }
     }
     // file names can match with +linenumber or :linenumber
-    QString postfix;
-    Link link = Link::fromString(entryFileName, true, &postfix);
+    const Link link = Link::fromString(entryFileName, true);
     regExp = createRegExp(link.targetFilePath.toString(), caseSensitivity_);
     if (!regExp.isValid())
         return {};
@@ -130,10 +129,11 @@ QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<LocatorF
         if (match.hasMatch()) {
             const MatchLevel level = matchLevelFor(match, file);
             const QString fullPath = dirInfo.filePath(file);
-            LocatorFilterEntry filterEntry(this, file, QString(fullPath + postfix));
+            LocatorFilterEntry filterEntry(this, file);
             filterEntry.filePath = FilePath::fromString(fullPath);
             filterEntry.highlightInfo = highlightInfo(match);
-
+            filterEntry.linkForEditor = Link(filterEntry.filePath, link.targetLine,
+                                             link.targetColumn);
             entries[int(level)].append(filterEntry);
         }
     }
@@ -142,9 +142,7 @@ QList<LocatorFilterEntry> FileSystemFilter::matchesFor(QFutureInterface<LocatorF
     const QString fullFilePath = dirInfo.filePath(entryFileName);
     const bool containsWildcard = expandedEntry.contains('?') || expandedEntry.contains('*');
     if (!containsWildcard && !QFileInfo::exists(fullFilePath) && dirInfo.exists()) {
-        LocatorFilterEntry createAndOpen(this,
-                                         Tr::tr("Create and Open \"%1\"").arg(expandedEntry),
-                                         fullFilePath);
+        LocatorFilterEntry createAndOpen(this, Tr::tr("Create and Open \"%1\"").arg(expandedEntry));
         createAndOpen.filePath = FilePath::fromString(fullFilePath);
         createAndOpen.extraInfo = FilePath::fromString(dirInfo.absolutePath()).shortNativePath();
         entries[int(MatchLevel::Normal)].append(createAndOpen);
@@ -170,13 +168,12 @@ void FileSystemFilter::accept(const LocatorFilterEntry &selection,
     } else {
         // Don't block locator filter execution with dialog
         QMetaObject::invokeMethod(EditorManager::instance(), [selection] {
-            const FilePath targetFile = FilePath::fromVariant(selection.internalData);
             if (!selection.filePath.exists()) {
                 if (CheckableMessageBox::shouldAskAgain(ICore::settings(), kAlwaysCreate)) {
                     CheckableMessageBox messageBox(ICore::dialogParent());
                     messageBox.setWindowTitle(Tr::tr("Create File"));
                     messageBox.setIcon(QMessageBox::Question);
-                    messageBox.setText(Tr::tr("Create \"%1\"?").arg(targetFile.shortNativePath()));
+                    messageBox.setText(Tr::tr("Create \"%1\"?").arg(selection.filePath.shortNativePath()));
                     messageBox.setCheckBoxVisible(true);
                     messageBox.setCheckBoxText(Tr::tr("Always create"));
                     messageBox.setChecked(false);
@@ -190,10 +187,10 @@ void FileSystemFilter::accept(const LocatorFilterEntry &selection,
                     if (messageBox.isChecked())
                         CheckableMessageBox::doNotAskAgain(ICore::settings(), kAlwaysCreate);
                 }
-                QFile file(targetFile.toString());
+                QFile file(selection.filePath.toString());
                 file.open(QFile::WriteOnly);
                 file.close();
-                VcsManager::promptToAdd(targetFile.absolutePath(), {targetFile});
+                VcsManager::promptToAdd(selection.filePath.absolutePath(), {selection.filePath});
             }
             BaseFileFilter::openEditorAt(selection);
         }, Qt::QueuedConnection);
diff --git a/src/plugins/coreplugin/locator/ilocatorfilter.h b/src/plugins/coreplugin/locator/ilocatorfilter.h
index 51657054c5d..e6b49ab5e9c 100644
--- a/src/plugins/coreplugin/locator/ilocatorfilter.h
+++ b/src/plugins/coreplugin/locator/ilocatorfilter.h
@@ -7,6 +7,7 @@
 
 #include <utils/filepath.h>
 #include <utils/id.h>
+#include <utils/link.h>
 
 #include <QFutureInterface>
 #include <QIcon>
@@ -67,7 +68,7 @@ struct LocatorFilterEntry
 
     LocatorFilterEntry(ILocatorFilter *fromFilter,
                        const QString &name,
-                       const QVariant &data,
+                       const QVariant &data = {},
                        std::optional<QIcon> icon = std::nullopt)
         : filter(fromFilter)
         , displayName(name)
@@ -93,7 +94,8 @@ struct LocatorFilterEntry
     Utils::FilePath filePath;
     /* highlighting support */
     HighlightInfo highlightInfo;
-
+    // Should be used only when accept() calls BaseFileFilter::openEditorAt()
+    std::optional<Utils::Link> linkForEditor;
     static bool compareLexigraphically(const Core::LocatorFilterEntry &lhs,
                                        const Core::LocatorFilterEntry &rhs)
     {
diff --git a/src/plugins/coreplugin/locator/javascriptfilter.cpp b/src/plugins/coreplugin/locator/javascriptfilter.cpp
index 175f0f44bc2..b97b054051f 100644
--- a/src/plugins/coreplugin/locator/javascriptfilter.cpp
+++ b/src/plugins/coreplugin/locator/javascriptfilter.cpp
@@ -60,7 +60,7 @@ QList<LocatorFilterEntry> JavaScriptFilter::matchesFor(
             entries.append({this, message, QVariant::fromValue(EngineAction::Abort)});
         } else {
             const QString expression = entry + " = " + result;
-            entries.append({this, expression, QVariant()});
+            entries.append({this, expression});
             entries.append({this, Tr::tr("Copy to clipboard: %1").arg(result), result});
             entries.append({this, Tr::tr("Copy to clipboard: %1").arg(expression), expression});
         }
diff --git a/src/plugins/coreplugin/locator/locatorsearchutils.cpp b/src/plugins/coreplugin/locator/locatorsearchutils.cpp
index d508629aa4d..77e460ad4f3 100644
--- a/src/plugins/coreplugin/locator/locatorsearchutils.cpp
+++ b/src/plugins/coreplugin/locator/locatorsearchutils.cpp
@@ -3,34 +3,33 @@
 
 #include "locatorsearchutils.h"
 
-#include <QSet>
-#include <QString>
-#include <QVariant>
+#include <utils/link.h>
+
+#include <unordered_set>
 
 void Core::Internal::runSearch(QFutureInterface<Core::LocatorFilterEntry> &future,
                                const QList<ILocatorFilter *> &filters, const QString &searchText)
 {
-    QSet<QString> alreadyAdded;
+    std::unordered_set<Utils::FilePath> addedCache;
     const bool checkDuplicates = (filters.size() > 1);
+    const auto duplicatesRemoved = [&](const QList<LocatorFilterEntry> &entries) {
+        if (!checkDuplicates)
+            return entries;
+        QList<LocatorFilterEntry> results;
+        results.reserve(entries.size());
+        for (const LocatorFilterEntry &entry : entries) {
+            const auto &link = entry.linkForEditor;
+            if (!link || addedCache.emplace(link->targetFilePath).second)
+                results.append(entry);
+        }
+        return results;
+    };
+
     for (ILocatorFilter *filter : filters) {
         if (future.isCanceled())
             break;
-
-        const QList<LocatorFilterEntry> filterResults = filter->matchesFor(future, searchText);
-        QVector<LocatorFilterEntry> uniqueFilterResults;
-        uniqueFilterResults.reserve(filterResults.size());
-        for (const LocatorFilterEntry &entry : filterResults) {
-            if (checkDuplicates) {
-                const QString stringData = entry.internalData.toString();
-                if (!stringData.isEmpty()) {
-                    if (alreadyAdded.contains(stringData))
-                        continue;
-                    alreadyAdded.insert(stringData);
-                }
-            }
-            uniqueFilterResults.append(entry);
-        }
-        if (!uniqueFilterResults.isEmpty())
-            future.reportResults(uniqueFilterResults);
+        const auto results = duplicatesRemoved(filter->matchesFor(future, searchText));
+        if (!results.isEmpty())
+            future.reportResults(results);
     }
 }
diff --git a/src/plugins/coreplugin/locator/locatorsearchutils.h b/src/plugins/coreplugin/locator/locatorsearchutils.h
index f1551eb9f2f..d863b580a67 100644
--- a/src/plugins/coreplugin/locator/locatorsearchutils.h
+++ b/src/plugins/coreplugin/locator/locatorsearchutils.h
@@ -9,8 +9,8 @@ namespace Core {
 namespace Internal {
 
 void CORE_EXPORT runSearch(QFutureInterface<LocatorFilterEntry> &future,
-                              const QList<ILocatorFilter *> &filters,
-                              const QString &searchText);
+                           const QList<ILocatorFilter *> &filters,
+                           const QString &searchText);
 
 } // namespace Internal
 } // namespace Core
diff --git a/src/plugins/coreplugin/locator/opendocumentsfilter.cpp b/src/plugins/coreplugin/locator/opendocumentsfilter.cpp
index 882f692329b..4037aad8e84 100644
--- a/src/plugins/coreplugin/locator/opendocumentsfilter.cpp
+++ b/src/plugins/coreplugin/locator/opendocumentsfilter.cpp
@@ -39,8 +39,7 @@ QList<LocatorFilterEntry> OpenDocumentsFilter::matchesFor(QFutureInterface<Locat
 {
     QList<LocatorFilterEntry> goodEntries;
     QList<LocatorFilterEntry> betterEntries;
-    QString postfix;
-    Link link = Link::fromString(entry, true, &postfix);
+    const Link link = Link::fromString(entry, true);
 
     const QRegularExpression regexp = createRegExp(link.targetFilePath.toString());
     if (!regexp.isValid())
@@ -56,10 +55,12 @@ QList<LocatorFilterEntry> OpenDocumentsFilter::matchesFor(QFutureInterface<Locat
         QString displayName = editorEntry.displayName;
         const QRegularExpressionMatch match = regexp.match(displayName);
         if (match.hasMatch()) {
-            LocatorFilterEntry filterEntry(this, displayName, QString(fileName + postfix));
+            LocatorFilterEntry filterEntry(this, displayName);
             filterEntry.filePath = FilePath::fromString(fileName);
             filterEntry.extraInfo = filterEntry.filePath.shortNativePath();
             filterEntry.highlightInfo = highlightInfo(match);
+            filterEntry.linkForEditor = Link(filterEntry.filePath, link.targetLine,
+                                             link.targetColumn);
             if (match.capturedStart() == 0)
                 betterEntries.append(filterEntry);
             else
diff --git a/src/plugins/coreplugin/locator/urllocatorfilter.cpp b/src/plugins/coreplugin/locator/urllocatorfilter.cpp
index 7ea2a2b87df..70651e74eea 100644
--- a/src/plugins/coreplugin/locator/urllocatorfilter.cpp
+++ b/src/plugins/coreplugin/locator/urllocatorfilter.cpp
@@ -176,7 +176,7 @@ QList<Core::LocatorFilterEntry> UrlLocatorFilter::matchesFor(
         if (future.isCanceled())
             break;
         const QString name = url.arg(entry);
-        Core::LocatorFilterEntry filterEntry(this, name, QVariant());
+        Core::LocatorFilterEntry filterEntry(this, name);
         filterEntry.highlightInfo = {int(name.lastIndexOf(entry)), int(entry.length())};
         entries.append(filterEntry);
     }
diff --git a/src/plugins/help/helpindexfilter.cpp b/src/plugins/help/helpindexfilter.cpp
index d544fe32547..fe1c34a48e0 100644
--- a/src/plugins/help/helpindexfilter.cpp
+++ b/src/plugins/help/helpindexfilter.cpp
@@ -86,7 +86,7 @@ QList<LocatorFilterEntry> HelpIndexFilter::matchesFor(QFutureInterface<LocatorFi
     QList<LocatorFilterEntry> entries;
     for (const QString &keyword : std::as_const(m_lastIndicesCache)) {
         const int index = keyword.indexOf(entry, 0, cs);
-        LocatorFilterEntry filterEntry(this, keyword, QVariant(), m_icon);
+        LocatorFilterEntry filterEntry(this, keyword, {}, m_icon);
         filterEntry.highlightInfo = {index, int(entry.length())};
         entries.append(filterEntry);
     }
diff --git a/src/plugins/macros/macrolocatorfilter.cpp b/src/plugins/macros/macrolocatorfilter.cpp
index 43d5bb3cf9a..43a9a489459 100644
--- a/src/plugins/macros/macrolocatorfilter.cpp
+++ b/src/plugins/macros/macrolocatorfilter.cpp
@@ -50,7 +50,7 @@ QList<Core::LocatorFilterEntry> MacroLocatorFilter::matchesFor(QFutureInterface<
         }
 
         if (index >= 0) {
-            Core::LocatorFilterEntry filterEntry(this, displayName, QVariant(), m_icon);
+            Core::LocatorFilterEntry filterEntry(this, displayName, {}, m_icon);
             filterEntry.extraInfo = description;
             filterEntry.highlightInfo = Core::LocatorFilterEntry::HighlightInfo(index, entry.length(), hDataType);
 
diff --git a/src/plugins/projectexplorer/projectexplorer.cpp b/src/plugins/projectexplorer/projectexplorer.cpp
index 622b18b31eb..91ca150008e 100644
--- a/src/plugins/projectexplorer/projectexplorer.cpp
+++ b/src/plugins/projectexplorer/projectexplorer.cpp
@@ -4362,10 +4362,8 @@ void RunConfigurationLocatorFilter::prepareSearch(const QString &entry)
     if (!target)
         return;
     for (auto rc : target->runConfigurations()) {
-        if (rc->displayName().contains(entry, Qt::CaseInsensitive)) {
-            Core::LocatorFilterEntry filterEntry(this, rc->displayName(), {});
-            m_result.append(filterEntry);
-        }
+        if (rc->displayName().contains(entry, Qt::CaseInsensitive))
+            m_result.append(LocatorFilterEntry(this, rc->displayName()));
     }
 }