lp1783392: Configurable debouncing timeout for WSearchLineEdit #1762

uklotzde · 2018-07-24T20:40:20Z

Add a configurable debouncing timeout for the search edit field. Depending on the size of their library users are now able to adjust this to mitigate the issues caused by locking the UI thread while searching.

Another user now has exactly reported the same issues that I am experiencing, i.e. Mixxx is almost unusable with bigger libraries and the default settings: https://bugs.launchpad.net/mixxx/+bug/1783392

uklotzde · 2018-10-13T11:25:34Z

After reverting the change of the debouncing timeout this PR actually fixes ...nothing.

Actual changes:

Simpler state management with less redundant code
Type-safe Qt 5 signal/slot connections
New signal for disabling the widget instead of using an implicit parameter with scattered code comments

uklotzde · 2018-10-14T07:26:45Z

The setting is now configurable, which might disqualify it from being included in the 2.2 beta.

daschuer · 2018-10-14T10:10:19Z

src/preferences/dialog/dlgpreflibrary.cpp


-#define MIXXX_ADDONS_URL "http://www.mixxx.org/wiki/doku.php/add-ons"


This macro is still in use. Is this an unrelated change, slipped in?

Slipped in. I didn't notice, because I'm already using a version without plugins for development. I've removed this obsolete #define now from master to prevent such mistakes in the future.

daschuer · 2018-10-14T11:16:25Z

src/preferences/dialog/dlgpreflibrarydlg.ui

        <property name="text">
-         <string>Edit metadata after clicking selected track</string>
+         <string>Incremental Search Delay:</string>


This might be misleading for me since "Incremental Search" for me is more an algorithm that works all the time in background guessing the final user result. Google has it in the search bar if the network bandwidth is good enough.
That would be great for Mixxx, but we are not there.

"Search after edit"
"Search As You Type Delay"
Or inverse
"Search inhibit time"

"Zero disables the feature" ??

Ok, I will revert back to the debouncing terminology also for the configuration setting for consistency.

For the text on the label we can use one of the proposed variants. Feedback by native speakers and especially non-developers welcome ;)

I don't want to allow a configurable timeout of 0 to disable this feature entirely. A maximum delay of almost 10 sec should be ok. But I've fixed the code to handle empty or invalid timeouts by not starting the timer.

uklotzde · 2018-10-14T11:54:12Z

The configurable timeout has proven to be very useful to me, because the optimal setting depends on both the size of your library as well as your personal typing speed and habits.

We have a similar fixed timeout for navigation within the side bar that is currently set to kPressedUntilClickedTimeoutMillis = 300. But this is not so sensitive, because when repeatedly clicking a button or turning an encoder you don't have to account for the delays while looking up different keys or thinking about how to finish your search.

daschuer

The timeout is bypassed if you delete the last character in the search box.
This also happens also in 2.1.
Can we have a minimal fix it there as well?

If I put the settingsPath on a USB HDD, I can reproduce the leaks. But they only happens if many tracks are shown or hidden due the search.
We may be able to explain it if all tracks are shown again, but why is this happening when tracks are hidden? Do we have a caching issue?

Disabling some columns doe does not change the issue by the way.

daschuer · 2018-10-14T20:28:42Z

.clang-format

@@ -0,0 +1,108 @@
+---


this file addition is unrelated to this PR.

daschuer · 2018-10-14T20:32:38Z

src/dialog/dlgdevelopertools.cpp

@@ -118,7 +118,7 @@ void DlgDeveloperTools::timerEvent(QTimerEvent* pEvent) {
    }
 }

-void DlgDeveloperTools::slotControlSearch(const QString& search) {
+void DlgDeveloperTools::slotControlSearch(QString search) {


Why is this change required? I think we should prefer passing strings by references, to avoid shared counting.o
In the queued connection case Qt makes the copy for us:
https://www.embeddeduse.com/2013/06/29/copied-or-not-copied-arguments-signals-slots/

daschuer · 2018-10-14T20:44:52Z

src/library/library.cpp

@@ -325,8 +325,8 @@ void Library::slotLoadTrackToPlayer(TrackPointer pTrack, QString group, bool pla
    emit(loadTrackToPlayer(pTrack, group, play));
 }

-void Library::slotRestoreSearch(const QString& text) {
-    emit(restoreSearch(text));
+void Library::slotRestoreSearch(QString text) {


Please keep passing by const reference here as well, if possible.

daschuer · 2018-10-14T20:47:31Z

src/widget/wlibrary.cpp

@@ -68,7 +68,7 @@ void WLibrary::switchToView(const QString& name) {
    }
 }

-void WLibrary::search(const QString& name) {
+void WLibrary::search(QString name) {


Please keep passing by const reference here as well, if possible.

daschuer · 2018-10-14T20:53:39Z

src/widget/wsearchlineedit.cpp

 }

-void WSearchLineEdit::slotTextChanged(const QString& text) {
+// slot
+void WSearchLineEdit::updateText(QString text) {


Please keep passing by const reference here as well, if possible.

src/widget/wsearchlineedit.cpp

daschuer · 2018-10-14T21:14:06Z

src/widget/wsearchlineedit.cpp

+    setAttribute(Qt::WA_MacShowFocusRect, false);
+}
+
+void WSearchLineEdit::updatePlaceholder(QString text) {


updatePlaceholder -> updateTextBox()
This function is hard to understand anyway. I would prefer to inline it.
Alternative it may call getSearchText() itselves.

daschuer · 2018-10-14T22:12:53Z

The setting is now configurable, which might disqualify it from being included in the 2.2 beta.

Unfortunately yes. Maybe the major issue can be fixed via solving the bypass issue.
A slightly longer delay may also fix some of the issues.

uklotzde · 2018-10-15T06:24:21Z

All intermediate, unwanted, or controversial changes have been reverted and the branch rebased on 2.2.

uklotzde · 2018-10-15T06:27:50Z

I cannot confirm that the timeout and timer is bypassed when deleting single characters. Deleting the last character with backspace triggers text changed events and the timer is reset as expected.

uklotzde · 2018-10-15T06:38:48Z

Target branch: One option would be to split the UI changes off from this PR and merge them separately into master. The new configuration remains undocumented, but could already be used by editing a single line in the config file manually.

We might need this configuration option to help fixing issues of users with big libraries. I expect at least some complaints once the new version has been released to a broader audience.

daschuer · 2018-10-15T15:27:14Z

If you think this option can be essential for 2.2 I think it is better to merge it including the GUI changes. A non translated essential feature is better than a hidden essentially feature. Both share the same regression risk.

Is this search bar issue a regression from 2.1? If yes, let's include it.

daschuer · 2018-10-15T15:28:48Z

Are you able to reproduce the deleting the last character GUI freezing?

uklotzde · 2018-10-15T20:14:22Z

Deleting the last characters works as like any other editing operation and restarts the timer as expected.

uklotzde · 2018-10-15T20:16:40Z

With Qt4 I was able to continue typing while the UI was frozen and ended up with the search phrase I typed. With Qt5 the characters typed while the UI is frozen are reordered in unforeseeable ways.

daschuer · 2018-10-15T22:37:51Z

The issue with the last character is "emit searchCleared();" during typing.
Can't we remove this signal completely and check for empty in onSearch?

I am also still unsure about the target branch.
Do we have other users suffering the character mix up issue?

uklotzde · 2018-10-15T22:44:31Z

I misunderstood your description "last character"! Ok, now I got it. Deleting the last character before the edit field becomes empty ;)

uklotzde · 2018-10-16T23:30:28Z

Another rebase to restore the lost disabledSearch() signal from the previous rebase
Activate timer upon deletion of last character in field
Simpler and more robust state handling

It still doesn't feel right, but I'm tired of improving this even more. I will only fix outstanding bugs. No backport to 2.1, this has already cost me much more time then planned.

daschuer · 2018-10-17T06:11:59Z

Thank you.
What do you mean by:

It still doesn't feel right?

We also don't have a conclusion if this as suits for 2.2. Do we know if this a Wayland issue?

I have tried to find something about the key stroke reordering on the web, but I haven't found anything.
Does it mean that this is induvidual an issue of your setup? Or is it sn issue that only happens with a certain Wayland and Qt version? Or is it a simple Mixxx bug?
What do you think?

uklotzde · 2018-10-17T06:47:07Z

"still not right": The underlying state machine with its side effects is still complicated. I'm not able to guarantee that all possible states and transitions are covered and work as expected. Testing with a large library that causes ui freezes and by varying the timeout were hopefully sufficient to reveal and fix most issues.

uklotzde · 2018-10-21T18:03:40Z

I've update the target to master/2.3.0, but I will keep this branch aligned with 2.2 until we know if this is an issue that affects multiple users.

daschuer · 2018-12-09T21:27:24Z

Let's merge this to 2.2 even if we have untranslated strings. Without it, Mixxx might be unusable for some users.

daschuer · 2018-12-09T21:28:13Z

Thank you!

uklotzde added this to the 2.2.0 milestone Jul 25, 2018

daschuer reviewed Oct 14, 2018

View reviewed changes

daschuer requested changes Oct 14, 2018

View reviewed changes

uklotzde changed the title ~~lp1783392: Improve debouncing for WSearchLineEdit~~ [WiP] lp1783392: Improve debouncing for WSearchLineEdit Oct 16, 2018

uklotzde changed the title ~~[WiP] lp1783392: Improve debouncing for WSearchLineEdit~~ lp1783392: Improve debouncing for WSearchLineEdit Oct 16, 2018

uklotzde changed the title ~~lp1783392: Improve debouncing for WSearchLineEdit~~ [WiP] lp1783392: Improve debouncing for WSearchLineEdit Oct 20, 2018

uklotzde changed the base branch from 2.2 to master October 21, 2018 18:02

uklotzde modified the milestones: 2.2.0, 2.3.0 Oct 21, 2018

uklotzde changed the title ~~[WiP] lp1783392: Improve debouncing for WSearchLineEdit~~ lp1783392: Improve debouncing for WSearchLineEdit Oct 23, 2018

uklotzde changed the base branch from master to 2.2 November 13, 2018 18:18

daschuer mentioned this pull request Nov 15, 2018

WSearchLineEdit #1903

Merged

uklotzde changed the title ~~lp1783392: Improve debouncing for WSearchLineEdit~~ [REBASE NEEDED] lp1783392: Improve debouncing for WSearchLineEdit Nov 18, 2018

uklotzde changed the title ~~[REBASE NEEDED] lp1783392: Improve debouncing for WSearchLineEdit~~ lp1783392: Configurable debouncing timeout for WSearchLineEdit Dec 3, 2018

uklotzde added 3 commits December 9, 2018 15:54

Load foreground color of WSearchLineEdit from skin

de5b107

Fix typo

e3f5c8e

Add configuration setting and ui for incremental search delay

af8adf7

daschuer approved these changes Dec 9, 2018

View reviewed changes

daschuer merged commit 35602e5 into mixxxdj:2.2 Dec 9, 2018

uklotzde deleted the lp1783392_wsearchlineedit branch December 26, 2018 23:50

This was referenced Aug 23, 2022

Search field: Keystrokes get mixed up #9392

Closed

double click Loads wrong track, after delte search and scroll #9519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lp1783392: Configurable debouncing timeout for WSearchLineEdit #1762

lp1783392: Configurable debouncing timeout for WSearchLineEdit #1762

uklotzde commented Jul 24, 2018 •

edited

Loading

uklotzde commented Oct 13, 2018

uklotzde commented Oct 14, 2018

daschuer Oct 14, 2018

uklotzde Oct 14, 2018

daschuer Oct 14, 2018

uklotzde Oct 14, 2018

uklotzde Oct 14, 2018

uklotzde commented Oct 14, 2018

daschuer left a comment

daschuer Oct 14, 2018

daschuer Oct 14, 2018

daschuer Oct 14, 2018

daschuer Oct 14, 2018

daschuer Oct 14, 2018

daschuer Oct 14, 2018

daschuer commented Oct 14, 2018

uklotzde commented Oct 15, 2018

uklotzde commented Oct 15, 2018 •

edited

Loading

uklotzde commented Oct 15, 2018

daschuer commented Oct 15, 2018

daschuer commented Oct 15, 2018 •

edited

Loading

uklotzde commented Oct 15, 2018

uklotzde commented Oct 15, 2018

daschuer commented Oct 15, 2018

uklotzde commented Oct 15, 2018

uklotzde commented Oct 16, 2018

daschuer commented Oct 17, 2018

uklotzde commented Oct 17, 2018

uklotzde commented Oct 21, 2018

daschuer commented Dec 9, 2018

daschuer commented Dec 9, 2018


		#define MIXXX_ADDONS_URL "http://www.mixxx.org/wiki/doku.php/add-ons"

lp1783392: Configurable debouncing timeout for WSearchLineEdit #1762

lp1783392: Configurable debouncing timeout for WSearchLineEdit #1762

Conversation

uklotzde commented Jul 24, 2018 • edited Loading

uklotzde commented Oct 13, 2018

uklotzde commented Oct 14, 2018

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

Choose a reason for hiding this comment

uklotzde commented Oct 14, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Oct 14, 2018

uklotzde commented Oct 15, 2018

uklotzde commented Oct 15, 2018 • edited Loading

uklotzde commented Oct 15, 2018

daschuer commented Oct 15, 2018

daschuer commented Oct 15, 2018 • edited Loading

uklotzde commented Oct 15, 2018

uklotzde commented Oct 15, 2018

daschuer commented Oct 15, 2018

uklotzde commented Oct 15, 2018

uklotzde commented Oct 16, 2018

daschuer commented Oct 17, 2018

uklotzde commented Oct 17, 2018

uklotzde commented Oct 21, 2018

daschuer commented Dec 9, 2018

daschuer commented Dec 9, 2018

uklotzde commented Jul 24, 2018 •

edited

Loading

uklotzde commented Oct 15, 2018 •

edited

Loading

daschuer commented Oct 15, 2018 •

edited

Loading