Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search: save queries across restarts, related tweaks & fixes #4458

Merged
merged 8 commits into from
Oct 29, 2021
13 changes: 13 additions & 0 deletions src/library/librarycontrol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,19 @@ LibraryControl::LibraryControl(Library* pLibrary)
m_pSearchbox->slotClearSearch();
}
});
m_pDeleteSearchQuery = std::make_unique<ControlPushButton>(
ConfigKey("[Library]", "delete_search_query"));
connect(m_pDeleteSearchQuery.get(),
&ControlPushButton::valueChanged,
this,
[this](double value) {
VERIFY_OR_DEBUG_ASSERT(m_pSearchbox) {
return;
}
if (value > 0.0) {
m_pSearchbox->slotDeleteCurrentItem();
}
});

/// Deprecated controls
m_pSelectNextTrack = std::make_unique<ControlPushButton>(ConfigKey("[Playlist]", "SelectNextTrack"));
Expand Down
1 change: 1 addition & 0 deletions src/library/librarycontrol.h
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ class LibraryControl : public QObject {
std::unique_ptr<ControlPushButton> m_pSelectHistoryPrev;
std::unique_ptr<ControlEncoder> m_pSelectHistorySelect;
std::unique_ptr<ControlPushButton> m_pClearSearch;
std::unique_ptr<ControlPushButton> m_pDeleteSearchQuery;

// Font sizes
std::unique_ptr<ControlPushButton> m_pFontSizeIncrement;
Expand Down
2 changes: 1 addition & 1 deletion src/skin/legacy/legacyskinparser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1299,7 +1299,7 @@ QWidget* LegacySkinParser::parseSearchBox(const QDomElement& node) {
WSearchLineEdit::kDefaultDebouncingTimeoutMillis);
WSearchLineEdit::setDebouncingTimeoutMillis(searchDebouncingTimeoutMillis);

WSearchLineEdit* pLineEditSearch = new WSearchLineEdit(m_pParent);
WSearchLineEdit* pLineEditSearch = new WSearchLineEdit(m_pParent, m_pConfig);
commonWidgetSetup(node, pLineEditSearch, false);
pLineEditSearch->setup(node, *m_pContext);

Expand Down
140 changes: 128 additions & 12 deletions src/widget/wsearchlineedit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ const QColor kDefaultBackgroundColor = QColor(0, 0, 0);

const QString kDisabledText = QStringLiteral("- - -");

const QString kSavedQueriesConfigGroup = QStringLiteral("[SearchQueries]");

constexpr int kClearButtonClearence = 1;

inline QString clearButtonStyleSheet(int pxPadding, Qt::LayoutDirection direction) {
Expand Down Expand Up @@ -74,9 +76,10 @@ void WSearchLineEdit::setDebouncingTimeoutMillis(int debouncingTimeoutMillis) {
s_debouncingTimeoutMillis = verifyDebouncingTimeoutMillis(debouncingTimeoutMillis);
}

WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
WSearchLineEdit::WSearchLineEdit(QWidget* pParent, UserSettingsPointer pConfig)
: QComboBox(pParent),
WBaseWidget(this),
m_pConfig(pConfig),
m_clearButton(make_parented<QToolButton>(this)) {
qRegisterMetaType<FocusWidget>("FocusWidget");
setAcceptDrops(false);
Expand Down Expand Up @@ -147,9 +150,15 @@ WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
clearButtonSize.width() + m_frameWidth + kClearButtonClearence,
layoutDirection()));

loadQueriesFromConfig();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause strange behaviour if there is more then 1 searchbar, but I don't know if this is even supported.
If there is a shared store for the queries there must be some sort of sync between the boxes and the config file, at least the object updated.

Copy link
Member Author

@ronso0 ronso0 Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a searchbar widget is created from the skin parser it's bound to the library widget.
So, if there is more than one searchbar only the last one is bound. didn't try that though. Edit both are bound and could work, but the queries are not sync'ed. Nothing to worry about since that is not a supported use case.

There's another WSearchLineEdit in the dev tools, but it's created without passing the settings pointer, thus no queries are stored for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we will not be doing a major redesign of the QWidgets library to add a second WSearchLineEdit, I am willing to accept something kinda hacky.


refreshState();
}

WSearchLineEdit::~WSearchLineEdit() {
saveQueriesInConfig();
}

void WSearchLineEdit::setup(const QDomNode& node, const SkinContext& context) {
auto backgroundColor = kDefaultBackgroundColor;
QString bgColorName;
Expand Down Expand Up @@ -219,17 +228,57 @@ void WSearchLineEdit::setup(const QDomNode& node, const SkinContext& context) {
tr("Use operators like bpm:115-128, artist:BooFar, -year:1990") +
"\n" + tr("For more information see User Manual > Mixxx Library") +
"\n\n" +
tr("Shortcut") + ": \n" + tr("Ctrl+F") + " " +
tr("Shortcuts") + ": \n" +
tr("Ctrl+F") + " " +
tr("Focus", "Give search bar input focus") + "\n" +
tr("Ctrl+Backspace") + " " +
tr("Clear input", "Clear the search bar input field") + "\n" +
tr("Ctrl+Space") + " " +
tr("Toggle search history",
"Shows/hides the search history entries") +
"\n" +
tr("Delete or Backspace") + " " + tr("Delete query from history") + "\n" +
tr("Esc") + " " + tr("Exit search", "Exit search bar and leave focus"));
}

void WSearchLineEdit::loadQueriesFromConfig() {
if (!m_pConfig) {
return;
}
const QList<ConfigKey> queryKeys =
m_pConfig->getKeysWithGroup(kSavedQueriesConfigGroup);
QSet<QString> queryStrings;
for (const auto& queryKey : queryKeys) {
const auto& queryString = m_pConfig->getValueString(queryKey);
if (queryString.isEmpty() || queryStrings.contains(queryString)) {
// Don't add duplicate and remove it from the config immediately
m_pConfig->remove(queryKey);
} else {
// Restore query
addItem(queryString);
queryStrings.insert(queryString);
}
}
}

void WSearchLineEdit::saveQueriesInConfig() {
if (!m_pConfig) {
return;
}
// Delete saved queries in case the list was cleared
const QList<ConfigKey> queryKeys =
m_pConfig->getKeysWithGroup(kSavedQueriesConfigGroup);
for (const auto& queryKey : queryKeys) {
m_pConfig->remove(queryKey);
}
// Store queries
for (int index = 0; index < count(); index++) {
m_pConfig->setValue(
ConfigKey(kSavedQueriesConfigGroup, QString::number(index)),
itemText(index));
}
}

void WSearchLineEdit::updateStyleMetrics() {
QStyleOptionComboBox styleArrow;
styleArrow.initFrom(this);
Expand Down Expand Up @@ -298,6 +347,13 @@ bool WSearchLineEdit::eventFilter(QObject* obj, QEvent* event) {
slotSaveSearch();
}
}
} else {
if (keyEvent->key() == Qt::Key_Backspace ||
keyEvent->key() == Qt::Key_Delete) {
// remove the highlighted item from the list
deleteSelectedListItem();
return true;
}
}
if (keyEvent->key() == Qt::Key_Enter) {
if (findCurrentTextIndex() == -1) {
Expand Down Expand Up @@ -335,6 +391,7 @@ void WSearchLineEdit::focusOutEvent(QFocusEvent* event) {
kLogger.trace()
<< "focusOutEvent";
#endif // ENABLE_TRACE_LOG
slotSaveSearch();
QComboBox::focusOutEvent(event);
emit searchbarFocusChange(FocusWidget::None);
if (m_debouncingTimer.isActive()) {
Expand Down Expand Up @@ -408,23 +465,32 @@ void WSearchLineEdit::slotTriggerSearch() {

/// saves the current query as selection
void WSearchLineEdit::slotSaveSearch() {
int cIndex = findCurrentTextIndex();
#if ENABLE_TRACE_LOG
kLogger.trace()
<< "save search. Index: "
<< cIndex;
#endif // ENABLE_TRACE_LOG
m_saveTimer.stop();
// entry already exists and is on top
if (cIndex == 0) {
if (currentText().isEmpty() || !isEnabled()) {
return;
}
if (!currentText().isEmpty() && isEnabled()) {
// we remove the existing item and add a new one at the top
if (cIndex != -1) {
removeItem(cIndex);
}
insertItem(0, currentText());
QString cText = currentText();
if (currentIndex() == -1) {
removeItem(-1);
}
// Check if the text is already listed
QSet<QString> querySet;
for (int index = 0; index < count(); index++) {
querySet.insert(itemText(index));
}
if (querySet.contains(cText)) {
// If query exists clear the box and use its index to set the currentIndex
int cIndex = findCurrentTextIndex();
setCurrentIndex(cIndex);
return;
} else {
// Else add it at the top
insertItem(0, cText);
setCurrentIndex(0);
while (count() > kMaxSearchEntries) {
removeItem(kMaxSearchEntries);
Expand All @@ -433,6 +499,9 @@ void WSearchLineEdit::slotSaveSearch() {
}

void WSearchLineEdit::slotMoveSelectedHistory(int steps) {
if (!isEnabled()) {
return;
}
int nIndex = currentIndex() + steps;
// at the top we manually wrap around to the last entry.
// at the bottom wrap-around happens automatically due to invalid nIndex.
Expand All @@ -443,6 +512,43 @@ void WSearchLineEdit::slotMoveSelectedHistory(int steps) {
m_saveTimer.stop();
}

void WSearchLineEdit::slotDeleteCurrentItem() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is misleading, because it deletes the current item or the highlighted item.
Since we gave the Chech in the calling code in the backspace case anyway I think the best solution is to make two functions and move the check into the valueChanged callback.
Alternative would be a better function name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I turned this into deleteSelectedListItem() and deleteSelectedComboboxItem(), and slotDeleteCurrentItem() which is connected to the CO which would pick one of the two depending on isVisible()

if (!isEnabled()) {
return;
}
if (view()->isVisible()) {
deleteSelectedListItem();
} else {
deleteSelectedComboboxItem();
}
}

void WSearchLineEdit::deleteSelectedComboboxItem() {
int cIndex = currentIndex();
if (cIndex == -1) {
return;
} else {
slotClearSearch();
QComboBox::removeItem(cIndex);
}
}

void WSearchLineEdit::deleteSelectedListItem() {
bool wasEmpty = currentIndex() == -1 ? true : false;
QComboBox::removeItem(view()->currentIndex().row());
// ToDo Resize the list to new item size
// Close the popup if all items were removed

// When an item is removed the combobox would pick a sibling and trigger a
// search. Avoid this if the box was empty when the popup was opened.
if (wasEmpty) {
QComboBox::setCurrentIndex(-1);
}
if (count() == 0) {
hidePopup();
}
}

void WSearchLineEdit::refreshState() {
#if ENABLE_TRACE_LOG
kLogger.trace()
Expand All @@ -464,6 +570,14 @@ void WSearchLineEdit::showPopup() {
setCurrentIndex(cIndex);
}
QComboBox::showPopup();
// When (empty) index -1 is selected in the combobox and the list view is opened
// index 0 is auto-set but not highlighted.
// Select first index manually (only in the list).
if (cIndex == -1) {
view()->selectionModel()->select(
view()->currentIndex(),
QItemSelectionModel::Select);
}
}

void WSearchLineEdit::updateEditBox(const QString& text) {
Expand Down Expand Up @@ -520,7 +634,9 @@ void WSearchLineEdit::slotClearSearch() {
kLogger.trace()
<< "slotClearSearch";
#endif // ENABLE_TRACE_LOG
DEBUG_ASSERT(isEnabled());
if (!isEnabled()) {
return;
}
// select the last entry as current before cleaning the text field
// so arrow keys will work as expected
setCurrentIndex(-1);
Expand Down
12 changes: 10 additions & 2 deletions src/widget/wsearchlineedit.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <QToolButton>

#include "library/library_decl.h"
#include "preferences/usersettings.h"
#include "util/parented_ptr.h"
#include "widget/wbasewidget.h"

Expand All @@ -26,8 +27,8 @@ class WSearchLineEdit : public QComboBox, public WBaseWidget {
static void setDebouncingTimeoutMillis(int debouncingTimeoutMillis);
virtual void showPopup() override;

explicit WSearchLineEdit(QWidget* pParent);
~WSearchLineEdit() override = default;
explicit WSearchLineEdit(QWidget* pParent, UserSettingsPointer pConfig = nullptr);
~WSearchLineEdit();

void setup(const QDomNode& node, const SkinContext& context);

Expand Down Expand Up @@ -55,6 +56,7 @@ class WSearchLineEdit : public QComboBox, public WBaseWidget {
/// entry in the history and executes the search.
/// The parameter specifies the distance in steps (positive/negative = downward/upward)
void slotMoveSelectedHistory(int steps);
void slotDeleteCurrentItem();

private slots:
void slotSetShortcutFocus();
Expand All @@ -78,6 +80,8 @@ class WSearchLineEdit : public QComboBox, public WBaseWidget {
void updateEditBox(const QString& text);
void updateClearButton(const QString& text);
void updateStyleMetrics();
void deleteSelectedComboboxItem();
void deleteSelectedListItem();

inline int findCurrentTextIndex() {
return findData(currentText(), Qt::DisplayRole);
Expand All @@ -88,6 +92,10 @@ class WSearchLineEdit : public QComboBox, public WBaseWidget {
// Update the displayed text without (re-)starting the timer
void setTextBlockSignals(const QString& text);

UserSettingsPointer m_pConfig;
void loadQueriesFromConfig();
void saveQueriesInConfig();

parented_ptr<QToolButton> const m_clearButton;

int m_frameWidth;
Expand Down