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

ronso0 · 2021-10-18T20:43:48Z

Store queries to mixxx.cfg when WSearchLineEdit is destroyed (shutdown or skin change), load when it's created.
(only for the library searchbar, not the control searchbar in the developer tools dialog)
Currently only the selected query is stored on skin change, others are lost.

Delete and Backspace in the popup delete the selected item
This allows cleaning up the history, which I consider a rare maintenance job thus I did not add a delete shortcut for the combobox view.
I also added a control [Library], delete_search_query to delete the current item from the popup or the combobox view.

tweaks & fixes:

The search queries popup holds indices 0..n, while the combobox also has index -1 (empty, "Search...").
When 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).
Avoid storing duplicate items when switching between library features.
Some searchbar COs could be triggered (accidentally) while the searchbar is disabled (AutoDJ, Rec, ...). Debug asserts were used which I replaced with regular tests in order to avoid unexpected behaviour in releases.

https://bugs.launchpad.net/mixxx/+bug/1943084
https://bugs.launchpad.net/mixxx/+bug/1947479

Be-ing · 2021-10-18T23:27:19Z

+102 −6
a small cost for a very useful feature

ronso0 · 2021-10-18T23:32:45Z

Yes, I was surprised how easy it is to save and load the queries. So easy that I'm afraid I overlooked a fundamental mistake...
But it works beautifully.

poelzi · 2021-10-18T23:59:16Z

Thank you for fixing the bug of disappearing queries, I was wondering where that came frome.
Lolz, I was thinking about saving the search queries, already thought about database models and API of the subsystem, you instead went the shortest of all path, well done. I rather like a simple system now then a perfectly designed subsystem in 3 years.
I would limit the number of entries saved in the config file to maybe 50 ? 500 ? over 9000 ?

Be-ing · 2021-10-19T00:03:15Z

I think this quick hack of using mixxx.cfg is adequate. AFAIK there isn't a technical reason this couldn't work as well as storing this data in the SQLite database, but this seems much simpler. When we replace the library with Aoide we'll probably have a completely different system for storing queries persistently. Considering how useful this feature is and the low complexity of the code being added here, I agree that it is worth it to have this now even if the user data may be incompatible with the future system.

poelzi · 2021-10-19T00:07:22Z

src/widget/wsearchlineedit.cpp

@@ -146,9 +149,15 @@ WSearchLineEdit::WSearchLineEdit(QWidget* pParent)
            clearButtonSize.width() + m_frameWidth + kClearButtonClearence,
            layoutDirection()));

+    loadQueriesFromConfig();


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.

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.

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.

ronso0 · 2021-10-19T00:18:21Z

Limiting the storage sounds reasonable. That could happen in WSearchLineEdit itself by setting maxCount().

daschuer · 2021-10-19T06:15:49Z

src/widget/wsearchlineedit.cpp

+        for (int index = 0; index < count(); index++) {
+            m_pConfig->setValue(
+                    ConfigKey(kSavedQueriesConfigGroup, QString::number(index)),
+                    this->itemText(index));


Suggested change

this->itemText(index));

itemText(index));

daschuer · 2021-10-19T06:24:16Z

src/widget/wsearchlineedit.cpp

@@ -297,6 +331,11 @@ bool WSearchLineEdit::eventFilter(QObject* obj, QEvent* event) {
                    slotSaveSearch();
                }
            }
+        } else {
+            if (keyEvent->key() == Qt::Key_Backspace) {


Please add a comment, how this works, it is not obvious when reading the code.
Doesn't it interfere with the backspace when editing the querry?

Ah got it.
The issue is that we have the check if the popup is visible twice.
Maybe two functions for each case makes it obvious.

daschuer

The is just a readability issue.
The rest looks good.
Thank you.

daschuer · 2021-10-19T06:28:47Z

src/widget/wsearchlineedit.cpp

@@ -439,6 +481,30 @@ void WSearchLineEdit::slotMoveSelectedHistory(int steps) {
    m_saveTimer.stop();
 }

+void WSearchLineEdit::slotDeleteCurrentItem() {


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.

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()

daschuer · 2021-10-19T06:34:49Z

src/widget/wsearchlineedit.cpp

@@ -297,6 +331,11 @@ bool WSearchLineEdit::eventFilter(QObject* obj, QEvent* event) {
                    slotSaveSearch();
                }
            }
+        } else {
+            if (keyEvent->key() == Qt::Key_Backspace) {


Ah got it.
The issue is that we have the check if the popup is visible twice.
Maybe two functions for each case makes it obvious.

ronso0 · 2021-10-19T11:37:02Z

Limiting the storage sounds reasonable. That could happen in WSearchLineEdit itself by setting maxCount().

What about 50?
I think that would allow the popup to be roughly as tall as the screen height.

ronso0 · 2021-10-19T16:12:23Z

@daschuer I implemented your suggestions. Please take a look.

btw I just discovered a bug (also in main):

select a search query
switch to another feature
select the previous query
switch back to the previous feature
the query is now twice in the list

I'll check if setDuplicatesEnabled(false) fixes this or if we need a duplicate filter in slotSaveSearch()

ronso0 · 2021-10-19T18:50:10Z

Limiting the storage sounds reasonable. That could happen in WSearchLineEdit itself by setting maxCount().

What about 50? I think that would allow the popup to be roughly as tall as the screen height.

ahemm, kMaxSearchEntries = 50; is already in place 😁

daschuer · 2021-10-19T19:21:17Z

Thank you.

I have just tested this and can confirm the issue:

Delete an entry form the pop up
Restart Mixxx
The last entry is listed twice.

Another thing is that I was tempted to use the del key instead of backspace more than once during the test. Can we add it as well?

ronso0 · 2021-10-19T19:45:00Z

Another thing is that I was tempted to use the del key instead of backspace more than once during the test. Can we add it as well?

Sure we could do that.
But considering Del is CMD + Backspace on macs that would add extra complexity (see #4330) I'm unsure how to proceed.

ronso0 · 2021-10-19T19:46:58Z

I fixed the duplicate bug, but I'll also need to fix the writing to the config to avoid duplicates there, probably wipe all entries before saving.

ronso0 · 2021-10-19T20:23:03Z

I fixed the duplicate bug, but I'll also need to fix the writing to the config to avoid duplicates there, probably wipe all entries before saving.

Done.

ronso0 · 2021-10-19T20:30:53Z

Further optimization would be to set QKey constants and use them for the tooltip and the key event filter.
Won't happen here though.

daschuer · 2021-10-20T06:28:28Z

But considering Del is CMD + Backspace on macs that would add extra complexity (see #4330) I'm unsure how to proceed.

I think it is sufficient to just add the del key.
It is probably unlikely that a Mac user uses CND + Backspace if Backspace allone works as well.

I have just checked how Firefox and Chrome works. Backspace starts immediately to edit the highlighted entry. It can be deleted by Shift+del and Shift +CMD+ Backspace..

Not sure if this patter is relevant for us.

src/widget/wsearchlineedit.cpp

ronso0 · 2021-10-20T23:39:46Z

suggestions addded & squashed.
Ready!

daschuer · 2021-10-21T05:42:05Z

Thank you, I will do a final check.

In general, you should not rebase a PR under review. This creates untested commits and makes it hard for the reviewer to review the changes compared to the last review. See: https://github.com/mixxxdj/mixxx/blob/main/CONTRIBUTING.md#all-contributors

ronso0 · 2021-10-21T12:05:05Z

Sorry, won't happen again. I'd like to avoid formatting one-line commits, and since I merged main I couldn't ammend anymore.

daschuer · 2021-10-21T12:59:26Z

Ok, no big deal in this case. Just ask before.

daschuer

This works like a charm. Thank you.

github-actions bot added the ui label Oct 18, 2021

ronso0 added library and removed ui labels Oct 18, 2021

github-actions bot added the ui label Oct 18, 2021

ronso0 changed the title ~~Search: Backspace deletes saved query~~ Search: save queries across restarts, related tweaks & fixes Oct 18, 2021

poelzi reviewed Oct 19, 2021

View reviewed changes

daschuer reviewed Oct 19, 2021

View reviewed changes

daschuer requested changes Oct 19, 2021

View reviewed changes

ronso0 force-pushed the search-delete-saved-queries branch from e1297fe to 966c0c0 Compare October 19, 2021 16:05

ronso0 force-pushed the search-delete-saved-queries branch 2 times, most recently from f47176a to e9055d9 Compare October 19, 2021 20:20

ronso0 force-pushed the search-delete-saved-queries branch from 45c0a1d to d13e847 Compare October 20, 2021 01:38

daschuer reviewed Oct 20, 2021

View reviewed changes

src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved

ronso0 added 2 commits October 21, 2021 01:10

Search: fix popup highlight

6554e66

Search: Del or Backspace in popup deletes query

ac0c5da

ronso0 added 3 commits October 21, 2021 01:16

Search: add ControlPushButton to remove selected query

d5f0398

Search: add isEnabled() tests

40f33d6

Search: save queries across restarts and when switching skins

d3f24fa

ronso0 force-pushed the search-delete-saved-queries branch from d13e847 to c78d9a4 Compare October 20, 2021 23:30

ronso0 added 3 commits October 21, 2021 01:37

Search: don't store duplicates in slotSaveSearch()

c3f328e

Search: also save search on focus out

c5be3f3

Search: don't update empty combobox when removing list items

a93b621

ronso0 force-pushed the search-delete-saved-queries branch from c78d9a4 to a93b621 Compare October 20, 2021 23:38

daschuer approved these changes Oct 29, 2021

View reviewed changes

daschuer merged commit 66be9a8 into mixxxdj:main Oct 29, 2021

ronso0 deleted the search-delete-saved-queries branch October 29, 2021 12:01

ronso0 added the changelog This PR should be included in the changelog label Dec 11, 2021

This was referenced Aug 23, 2022

store search queries across restarts, allow to delete queries #10517

Closed

saved search queries are cleared after skin change/reload #10561

Closed

ronso0 mentioned this pull request Dec 16, 2022

Smart crates / save and recall search queries #5575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronso0 commented Oct 18, 2021 •

edited

Loading

Be-ing commented Oct 18, 2021

ronso0 commented Oct 18, 2021

poelzi commented Oct 18, 2021

Be-ing commented Oct 19, 2021

poelzi Oct 19, 2021

ronso0 Oct 19, 2021 •

edited

Loading

Be-ing Oct 19, 2021

ronso0 commented Oct 19, 2021

daschuer Oct 19, 2021

daschuer Oct 19, 2021

daschuer Oct 19, 2021

daschuer left a comment

daschuer Oct 19, 2021

ronso0 Oct 19, 2021

daschuer Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021 •

edited

Loading

ronso0 commented Oct 19, 2021

daschuer commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

daschuer commented Oct 20, 2021 •

edited

Loading

ronso0 commented Oct 20, 2021

daschuer commented Oct 21, 2021 •

edited

Loading

ronso0 commented Oct 21, 2021

daschuer commented Oct 21, 2021

daschuer left a comment

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

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

Conversation

ronso0 commented Oct 18, 2021 • edited Loading

Be-ing commented Oct 18, 2021

ronso0 commented Oct 18, 2021

poelzi commented Oct 18, 2021

Be-ing commented Oct 19, 2021

Choose a reason for hiding this comment

ronso0 Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021 • edited Loading

ronso0 commented Oct 19, 2021

daschuer commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

ronso0 commented Oct 19, 2021

daschuer commented Oct 20, 2021 • edited Loading

ronso0 commented Oct 20, 2021

daschuer commented Oct 21, 2021 • edited Loading

ronso0 commented Oct 21, 2021

daschuer commented Oct 21, 2021

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Oct 18, 2021 •

edited

Loading

ronso0 Oct 19, 2021 •

edited

Loading

ronso0 commented Oct 19, 2021 •

edited

Loading

daschuer commented Oct 20, 2021 •

edited

Loading

daschuer commented Oct 21, 2021 •

edited

Loading