Merge pull request #2748 from uklotzde/lp1876202_wsearchlineedit

Lp1876202: Fix WSearchLineEdit state machine

5 files changed, 210 insertions, 146 deletions

diff --git a/src/library/library.cpp b/src/library/library.cpp
index a685199bb9..136c92c4cb 100644
--- a/src/library/library.cpp
+++ b/src/library/library.cpp
@@ -236,11 +236,11 @@ void Library::bindSearchboxWidget(WSearchLineEdit* pSearchboxWidget) {
     connect(this,
             &Library::disableSearch,
             pSearchboxWidget,
-            &WSearchLineEdit::disableSearch);
+            &WSearchLineEdit::slotDisableSearch);
     connect(this,
             &Library::restoreSearch,
             pSearchboxWidget,
-            &WSearchLineEdit::restoreSearch);
+            &WSearchLineEdit::slotRestoreSearch);
     connect(this,
             &Library::setTrackTableFont,
             pSearchboxWidget,
diff --git a/src/library/librarycontrol.cpp b/src/library/librarycontrol.cpp
index ca48351c92..d1e9d6579f 100644
--- a/src/library/librarycontrol.cpp
+++ b/src/library/librarycontrol.cpp
@@ -333,9 +333,9 @@ void LibraryControl::bindSearchboxWidget(WSearchLineEdit* pSearchbox) {
     }
     m_pSearchbox = pSearchbox;
     connect(this,
-            &LibraryControl::clearSearch,
+            &LibraryControl::clearSearchIfClearButtonHasFocus,
             m_pSearchbox,
-            &WSearchLineEdit::clearSearch);
+            &WSearchLineEdit::slotClearSearchIfClearButtonHasFocus);
     connect(m_pSearchbox,
             &WSearchLineEdit::destroyed,
             this,
@@ -621,9 +621,8 @@ void LibraryControl::slotGoToItem(double v) {
     }
 
     // Clear the search if the searchbox has focus
-    if (m_pSearchbox->clearBtnHasFocus()) {
-        return emit clearSearch();
-    }
+    emit clearSearchIfClearButtonHasFocus();
+
     // TODO(xxx) instead of remote control the widgets individual, we should
     // translate this into Alt+Return and handle it at each library widget
     // individual https://bugs.launchpad.net/mixxx/+bug/1758618
diff --git a/src/library/librarycontrol.h b/src/library/librarycontrol.h
index a6cb40a089..95967fbb90 100644
--- a/src/library/librarycontrol.h
+++ b/src/library/librarycontrol.h
@@ -46,7 +46,7 @@ class LibraryControl : public QObject {
     void bindSearchboxWidget(WSearchLineEdit* pSearchbox);
 
   signals:
-    void clearSearch();
+    void clearSearchIfClearButtonHasFocus();
 
   public slots:
     // Deprecated navigation slots
diff --git a/src/widget/wsearchlineedit.cpp b/src/widget/wsearchlineedit.cpp
index f8c61bc901..a01e0cfbe3 100644
--- a/src/widget/wsearchlineedit.cpp
+++ b/src/widget/wsearchlineedit.cpp
@@ -11,16 +11,26 @@
 #include "util/assert.h"
 #include "util/logger.h"
 
+#define ENABLE_TRACE_LOG false
+
 namespace {
 
 const mixxx::Logger kLogger("WSearchLineEdit");
 
-const QColor kDefaultForegroundColor = QColor(0, 0, 0);
-const QColor kDefaultBackgroundColor = QColor(255, 255, 255);
+const QColor kDefaultBackgroundColor = QColor(0, 0, 0);
+
+const QString kEmptySearch = QStringLiteral("");
 
-const QString kEmptySearch = "";
+const QString kDisabledText = QStringLiteral("- - -");
 
-const QString kDisabledText = "- - -";
+const QString kClearButtonName = QStringLiteral("SearchClearButton");
+
+inline QString clearButtonStyleSheet(int pxPaddingRight) {
+    DEBUG_ASSERT(pxPaddingRight >= 0);
+    return QString(
+            QStringLiteral("QLineEdit { padding-right: %1px; }"))
+            .arg(pxPaddingRight);
+}
 
 int verifyDebouncingTimeoutMillis(int debouncingTimeoutMillis) {
     VERIFY_OR_DEBUG_ASSERT(debouncingTimeoutMillis >= WSearchLineEdit::kMinDebouncingTimeoutMillis) {
@@ -54,23 +64,24 @@ void WSearchLineEdit::setDebouncingTimeoutMillis(int debouncingTimeoutMillis) {
 WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
     : QLineEdit(pParent),
       WBaseWidget(this),
-      m_clearButton(new QToolButton(this)),
-      m_foregroundColor(kDefaultForegroundColor),
-      m_state(State::Inactive) {
+      m_clearButton(make_parented<QToolButton>(this)) {
     DEBUG_ASSERT(kEmptySearch.isEmpty());
     DEBUG_ASSERT(!kEmptySearch.isNull());
 
     setAcceptDrops(false);
 
+    //: Shown in the library search bar when it is empty.
+    setPlaceholderText(tr("Search..."));
+
     m_clearButton->setCursor(Qt::ArrowCursor);
-    m_clearButton->setObjectName("SearchClearButton");
+    m_clearButton->setObjectName(kClearButtonName);
     // Assume the qss border is at least 1px wide
     m_frameWidth = 1;
     m_clearButton->hide();
     connect(m_clearButton,
             &QAbstractButton::clicked,
             this,
-            &WSearchLineEdit::clearSearch);
+            &WSearchLineEdit::slotClearSearch);
 
     // This prevents the searchbox from being focused by Tab key (real or emulated)
     // so it is skipped when using the library controls 'MoveFocus[...]'
@@ -80,7 +91,7 @@ WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
     connect(setFocusShortcut,
             &QShortcut::activated,
             this,
-            &WSearchLineEdit::setShortcutFocus);
+            &WSearchLineEdit::slotSetShortcutFocus);
 
     // Set up a timer to search after a few hundred milliseconds timeout.  This
     // stops us from thrashing the database if you type really fast.
@@ -88,24 +99,28 @@ WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
     connect(&m_debouncingTimer,
             &QTimer::timeout,
             this,
-            &WSearchLineEdit::triggerSearch);
+            &WSearchLineEdit::slotTriggerSearch);
     connect(this,
             &QLineEdit::textChanged,
             this,
-            &WSearchLineEdit::updateText);
+            &WSearchLineEdit::slotTextChanged);
 
-    // When you hit enter, it will trigger the search.
+    // When you hit enter, it will trigger or clear the search.
     connect(this,
             &QLineEdit::returnPressed,
             this,
-            &WSearchLineEdit::triggerSearch);
+            [this] {
+                if (!slotClearSearchIfClearButtonHasFocus()) {
+                    slotTriggerSearch();
+                }
+            });
 
     QSize clearButtonSize = m_clearButton->sizeHint();
+
     // Ensures the text does not obscure the clear image.
-    setStyleSheet(QString("QLineEdit { padding-right: %1px; } ")
-                          .arg(clearButtonSize.width() + m_frameWidth + 1));
+    setStyleSheet(clearButtonStyleSheet(clearButtonSize.width() + m_frameWidth + 1));
 
-    showPlaceholder();
+    refreshState();
 }
 
 void WSearchLineEdit::setup(const QDomNode& node, const SkinContext& context) {
@@ -128,36 +143,44 @@ void WSearchLineEdit::setup(const QDomNode& node, const SkinContext& context) {
             << backgroundColor;
 
     const auto defaultForegroundColor =
-            QColor(
+            QColor::fromRgb(
                     255 - backgroundColor.red(),
                     255 - backgroundColor.green(),
                     255 - backgroundColor.blue());
-    m_foregroundColor = defaultForegroundColor;
+    auto foregroundColor = defaultForegroundColor;
     QString fgColorName;
     if (context.hasNodeSelectString(node, "FgColor", &fgColorName)) {
         auto namedColor = QColor(fgColorName);
         if (namedColor.isValid()) {
-            m_foregroundColor = namedColor;
+            foregroundColor = namedColor;
         } else {
             kLogger.warning()
                     << "Failed to parse foreground color"
                     << fgColorName;
         }
     }
-    m_foregroundColor = WSkinColor::getCorrectColor(m_foregroundColor);
-    VERIFY_OR_DEBUG_ASSERT(m_foregroundColor != backgroundColor) {
+    foregroundColor = WSkinColor::getCorrectColor(foregroundColor);
+    VERIFY_OR_DEBUG_ASSERT(foregroundColor != backgroundColor) {
         kLogger.warning()
                 << "Invisible foreground color - using default color as fallback";
-        m_foregroundColor = defaultForegroundColor;
+        foregroundColor = defaultForegroundColor;
     }
     kLogger.debug()
             << "Foreground color:"
-            << m_foregroundColor;
+            << foregroundColor;
 
     QPalette pal = palette();
-    DEBUG_ASSERT(backgroundColor != m_foregroundColor);
+    DEBUG_ASSERT(backgroundColor != foregroundColor);
     pal.setBrush(backgroundRole(), backgroundColor);
-    pal.setBrush(foregroundRole(), m_foregroundColor);
+    pal.setBrush(foregroundRole(), foregroundColor);
+#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0)
+    auto placeholderColor = foregroundColor;
+    placeholderColor.setAlpha(placeholderColor.alpha() * 3 / 4); // 75% opaque
+    kLogger.debug()
+            << "Placeholder color:"
+            << placeholderColor;
+    pal.setBrush(QPalette::PlaceholderText, placeholderColor);
+#endif
     setPalette(pal);
 
     m_clearButton->setToolTip(tr("Clear input") + "\n" +
@@ -189,7 +212,7 @@ void WSearchLineEdit::resizeEvent(QResizeEvent* e) {
         m_clearButton->setIconSize(newSize);
         // Note(ronso0): For some reason this ensures the search text
         // is being displayed after skin change/reload.
-        updateEditBox(getSearchText());
+        refreshState();
     }
     int top = rect().top() + m_frameWidth;
     if (layoutDirection() == Qt::LeftToRight) {
@@ -200,7 +223,8 @@ void WSearchLineEdit::resizeEvent(QResizeEvent* e) {
 }
 
 QString WSearchLineEdit::getSearchText() const {
-    if (isEnabled() && (m_state == State::Active)) {
+    if (isEnabled()) {
+        DEBUG_ASSERT(!text().isNull());
         return text();
     } else {
         return QString();
@@ -208,95 +232,137 @@ QString WSearchLineEdit::getSearchText() const {
 }
 
 void WSearchLineEdit::focusInEvent(QFocusEvent* event) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "focusInEvent";
+#endif // ENABLE_TRACE_LOG
     QLineEdit::focusInEvent(event);
-    showSearchText(getSearchText());
 }
 
 void WSearchLineEdit::focusOutEvent(QFocusEvent* event) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "focusOutEvent";
+#endif // ENABLE_TRACE_LOG
     QLineEdit::focusOutEvent(event);
-    updateEditBox(getSearchText());
+    if (m_debouncingTimer.isActive()) {
+        // Trigger a pending search before leaving the edit box.
+        // Otherwise the entered text might be ignored and get lost
+        // due to the debouncing timeout!
+        slotTriggerSearch();
+    }
 }
 
-// slot
-void WSearchLineEdit::disableSearch() {
-    restoreSearch(QString());
+void WSearchLineEdit::setTextBlockSignals(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "setTextBlockSignals"
+            << text;
+#endif // ENABLE_TRACE_LOG
+    blockSignals(true);
+    setText(text);
+    blockSignals(false);
+}
+
+void WSearchLineEdit::slotDisableSearch() {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "slotDisableSearch";
+#endif // ENABLE_TRACE_LOG
+    if (!isEnabled()) {
+        return;
+    }
+    setTextBlockSignals(kDisabledText);
+    setEnabled(false);
+}
+
+void WSearchLineEdit::enableSearch(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "enableSearch"
+            << text;
+#endif // ENABLE_TRACE_LOG
+    if (isEnabled()) {
+        return;
+    }
+    // Set enabled BEFORE updating the edit box!
+    setEnabled(true);
+    updateEditBox(text);
 }
 
-// slot
-void WSearchLineEdit::restoreSearch(const QString& text) {
+void WSearchLineEdit::slotRestoreSearch(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "slotRestoreSearch"
+            << text;
+#endif // ENABLE_TRACE_LOG
     if (text.isNull()) {
-        // disable
-        setEnabled(false);
-        setText(kDisabledText);
+        slotDisableSearch();
     } else {
-        setEnabled(true);
-        // Updating the placeholder implicitly updates the text and the clear button
-        updateEditBox(text);
+        enableSearch(text);
     }
 }
 
-// slot
-void WSearchLineEdit::triggerSearch() {
+void WSearchLineEdit::slotTriggerSearch() {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "slotTriggerSearch"
+            << getSearchText();
+#endif // ENABLE_TRACE_LOG
+    DEBUG_ASSERT(isEnabled());
     m_debouncingTimer.stop();
     emit search(getSearchText());
 }
 
-void WSearchLineEdit::showPlaceholder() {
-    DEBUG_ASSERT(isEnabled());
-
-    // Deactivate text change listener
-    m_state = State::Inactive;
-
-    setText(tr("Search...", "noun"));
-
-    QPalette pal = palette();
-    pal.setColor(foregroundRole(), Qt::lightGray);
-    setPalette(pal);
+void WSearchLineEdit::refreshState() {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "refreshState";
+#endif // ENABLE_TRACE_LOG
+    if (isEnabled()) {
+        enableSearch(getSearchText());
+    } else {
+        slotDisableSearch();
+    }
 }
 
-void WSearchLineEdit::showSearchText(const QString& text) {
+void WSearchLineEdit::updateEditBox(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "updateEditBox"
+            << text;
+#endif // ENABLE_TRACE_LOG
     DEBUG_ASSERT(isEnabled());
 
-    // Reactivate text change listener
-    m_state = State::Active;
-
-    // Update the displayed text without (re-)starting the timer
-    blockSignals(true);
-    if (text.isNull()) {
-        setText(kEmptySearch);
+    if (text.isEmpty()) {
+        setTextBlockSignals(kEmptySearch);
     } else {
-        setText(text);
+        setTextBlockSignals(text);
     }
-    blockSignals(false);
-
     updateClearButton(text);
 
-    QPalette pal = palette();
-    pal.setColor(foregroundRole(), m_foregroundColor);
-    setPalette(pal);
-
     // This gets rid of the blue mac highlight.
     setAttribute(Qt::WA_MacShowFocusRect, false);
 }
 
-void WSearchLineEdit::updateEditBox(const QString& text) {
+void WSearchLineEdit::updateClearButton(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "updateClearButton"
+            << text;
+#endif // ENABLE_TRACE_LOG
+    DEBUG_ASSERT(isEnabled());
+
     if (text.isEmpty()) {
-        showPlaceholder();
+        // Disable while placeholder is shown
+        m_clearButton->setVisible(false);
+        // no right padding
+        setStyleSheet(clearButtonStyleSheet(0));
     } else {
-        showSearchText(text);
-    }
-}
-
-void WSearchLineEdit::updateClearButton(const QString& text) {
-    if (!text.isEmpty() && (m_state == State::Active)) {
+        // Enable otherwise
         m_clearButton->setVisible(true);
         // make sure the text won't be drawn behind the Clear button icon
-        setStyleSheet(QString("QLineEdit { padding-right: %1px; } ")
-                              .arg(m_innerHeight + m_frameWidth));
-    } else {
-        m_clearButton->setVisible(false);
-        // no right padding
-        setStyleSheet(QString("QLineEdit { padding-right: 0px; } "));
+        setStyleSheet(clearButtonStyleSheet(m_innerHeight + m_frameWidth));
     }
 }
 
@@ -307,43 +373,54 @@ bool WSearchLineEdit::event(QEvent* pEvent) {
     return QLineEdit::event(pEvent);
 }
 
-// slot
-bool WSearchLineEdit::clearBtnHasFocus() const {
-    return m_clearButton->hasFocus();
-}
-
-// slot
-void WSearchLineEdit::clearSearch() {
-    DEBUG_ASSERT(m_state == State::Active);
-    setText(kEmptySearch);
+void WSearchLineEdit::slotClearSearch() {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "slotClearSearch";
+#endif // ENABLE_TRACE_LOG
+    DEBUG_ASSERT(isEnabled());
     // Clearing the edit field will engage the debouncing timer
     // and gives the user the chance for entering a new search
     // before returning the whole (and probably huge) library.
     // No need to manually trigger a search at this point!
     // See also: https://bugs.launchpad.net/mixxx/+bug/1635087
+    setText(kEmptySearch);
+    // Refocus the edit field
+    setFocus(Qt::OtherFocusReason);
 }
 
-// slot
-void WSearchLineEdit::updateText(const QString& text) {
-    if (isEnabled() && (m_state == State::Active)) {
-        updateClearButton(text);
-        DEBUG_ASSERT(m_debouncingTimer.isSingleShot());
-        if (s_debouncingTimeoutMillis > 0) {
-            m_debouncingTimer.start(s_debouncingTimeoutMillis);
-        } else {
-            // Deactivate the timer if the timeout is invalid.
-            // Disabling the timer permanently by setting the timeout
-            // to an invalid value is an expected and valid use case.
-            m_debouncingTimer.stop();
-        }
+bool WSearchLineEdit::slotClearSearchIfClearButtonHasFocus() {
+    if (!m_clearButton->hasFocus()) {
+        return false;
+    }
+    slotClearSearch();
+    return true;
+}
+
+void WSearchLineEdit::slotTextChanged(const QString& text) {
+#if ENABLE_TRACE_LOG
+    kLogger.trace()
+            << "slotTextChanged"
+            << text;
+#endif // ENABLE_TRACE_LOG
+    m_debouncingTimer.stop();
+    if (!isEnabled()) {
+        setTextBlockSignals(kDisabledText);
+        return;
+    }
+    updateClearButton(text);
+    DEBUG_ASSERT(m_debouncingTimer.isSingleShot());
+    if (s_debouncingTimeoutMillis > 0) {
+        m_debouncingTimer.start(s_debouncingTimeoutMillis);
     } else {
-        updateClearButton(QString());
-        m_debouncingTimer.stop();
+        // Don't (re-)activate the timer if the timeout is invalid.
+        // Disabling the timer permanently by setting the timeout
+        // to an invalid value is an expected and valid use case.
+        DEBUG_ASSERT(!m_debouncingTimer.isActive());
     }
 }
 
-// slot
-void WSearchLineEdit::setShortcutFocus() {
+void WSearchLineEdit::slotSetShortcutFocus() {
     setFocus(Qt::ShortcutFocusReason);
 }
 
diff --git a/src/widget/wsearchlineedit.h b/src/widget/wsearchlineedit.h
index 4c9db3d53e..f3807ce003 100644
--- a/src/widget/wsearchlineedit.h
+++ b/src/widget/wsearchlineedit.h
@@ -1,12 +1,12 @@
 #pragma once
 
-#include <QColor>
 #include <QDomNode>
 #include <QEvent>
 #include <QLineEdit>
 #include <QTimer>
 #include <QToolButton>
 
+#include "util/parented_ptr.h"
 #include "widget/wbasewidget.h"
 
 class SkinContext;
@@ -14,24 +14,11 @@ class SkinContext;
 class WSearchLineEdit : public QLineEdit, public WBaseWidget {
     Q_OBJECT
   public:
-    enum class State {
-        Inactive,
-        Active,
-    };
-
     // Delay for triggering a search while typing
     static constexpr int kMinDebouncingTimeoutMillis = 100;
     static constexpr int kDefaultDebouncingTimeoutMillis = 300;
     static constexpr int kMaxDebouncingTimeoutMillis = 9999;
 
-    // Use 'active' property to apply an alternative style to the
-    // placeholder text
-    Q_PROPERTY(bool active READ isActive);
-
-    bool isActive() const {
-        return m_state == State::Active ? true : false;
-    }
-
     // TODO(XXX): Replace with a public slot
     static void setDebouncingTimeoutMillis(int debouncingTimeoutMillis);
 
@@ -50,17 +37,19 @@ class WSearchLineEdit : public QLineEdit, public WBaseWidget {
     void search(const QString& text);
 
   public slots:
-    void restoreSearch(const QString& text);
-    void disableSearch();
     void slotSetFont(const QFont& font);
-    bool clearBtnHasFocus() const;
-    void clearSearch();
+
+    void slotRestoreSearch(const QString& text);
+    void slotDisableSearch();
+
+    void slotClearSearch();
+    bool slotClearSearchIfClearButtonHasFocus();
 
   private slots:
-    void setShortcutFocus();
-    void updateText(const QString& text);
+    void slotSetShortcutFocus();
+    void slotTextChanged(const QString& text);
 
-    void triggerSearch();
+    void slotTriggerSearch();
 
   private:
     // TODO(XXX): This setting shouldn't be static and the widget
@@ -70,22 +59,21 @@ class WSearchLineEdit : public QLineEdit, public WBaseWidget {
     // configuration value changes.
     static int s_debouncingTimeoutMillis;
 
-    void showPlaceholder();
-    void showSearchText(const QString& text);
-    void updateEditBox(const QString& text);
+    void refreshState();
 
+    void enableSearch(const QString& text);
+    void updateEditBox(const QString& text);
     void updateClearButton(const QString& text);
 
     QString getSearchText() const;
 
-    QToolButton* const m_clearButton;
+    // Update the displayed text without (re-)starting the timer
+    void setTextBlockSignals(const QString& text);
+
+    parented_ptr<QToolButton> const m_clearButton;
 
     int m_frameWidth;
     int m_innerHeight;
 
-    QColor m_foregroundColor;
-
     QTimer m_debouncingTimer;
-
-    State m_state;
 };