No longer trigger unneccessary searches om left and right cursor keys #11289

daschuer · 2023-02-20T11:42:39Z

This fixes #11287, a regression from
e25071d

src/widget/wsearchlineedit.cpp

ronso0 · 2023-02-20T13:12:05Z

On second thought: let's not make pressing Right trigger a search.
IMO that should just accept the completion -- triggering the search should be up to the user (Enter) or the debounce timer.

ronso0 · 2023-02-20T13:21:56Z

Otoh that would delay triggering the search (debounce timer is restarted on each keystroke).
But maybe that's even okay since moving the cursor somehow implies the user is still refining the search phrase?

ronso0 · 2023-02-20T13:33:37Z

On second thought: let's not make pressing Right trigger a search. IMO that should just accept the completion -- triggering the search should be up to the user (Enter) or the debounce timer.

ed10381

uklotzde · 2023-02-20T13:57:16Z

Otoh that would delay triggering the search (debounce timer is restarted on each keystroke). But maybe that's even okay since moving the cursor somehow implies the user is still refining the search phrase?

This is the expected behavior!

uklotzde · 2023-02-20T18:11:11Z

src/widget/wsearchlineedit.cpp

        QLineEdit* pEdit = lineEdit();
-        bool hadSelectedText = pEdit->hasSelectedText();
+        bool hadSelectedText = pEdit ? pEdit->hasSelectedText() : false;


These null checks do not help and only complicate the code. We expect that there is always a line QLineEdit. Just invoke lineEdit() without using any mutable, local variables.

There is no guarantee that line edit is not null:
https://github.com/qt/qtbase/blob/e0a5915f88a780ba0791bce5ed042e92036a4900/src/widgets/widgets/qcombobox.cpp#L1697
Better safe than sorry.

nup, I figured it's the other way around:
if it's editable then there's always a QLineEdit.
And since the editable state is not changed there will always be a lineEdit()
https://github.com/mixxxdj/mixxx/pull/11289/files#diff-85c225cd63139457cde84ddb0d2f69c854c16f8254672cd19587fd25dd755c62L94

~~Then you need to use VERIFY_OR_DEBUG_ASSERT~~ Then please simplify the code, i.e. invoke lineEdit() when needed and use the resulting pointer directly.

For sake of completeness, if isEditable is true then there is a QLineEdit, otherwise lineEdit() is a nullptr:

void QComboBox::setEditable(bool editable) { Q_D(QComboBox); if (isEditable() == editable) return; QStyleOptionComboBox opt; initStyleOption(&opt); if (editable) { if (style()->styleHint(QStyle::SH_ComboBox_Popup, &opt, this)) { d->viewContainer()->updateScrollers(); view()->setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded); } QLineEdit *le = new QLineEdit(this); le->setPalette(palette()); setLineEdit(le); } else { if (style()->styleHint(QStyle::SH_ComboBox_Popup, &opt, this)) { d->viewContainer()->updateScrollers(); view()->setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); } setAttribute(Qt::WA_InputMethodEnabled, false); d->lineEdit->hide(); d->lineEdit->deleteLater(); d->lineEdit = nullptr; } d->updateDelegate(); d->updateFocusPolicy(); d->viewContainer()->updateTopBottomMargin(); if (!testAttribute(Qt::WA_Resized)) adjustSize(); }

daschuer · 2023-02-20T18:29:08Z

I did some usability test and it works now nice even with a looong delay of 10 s

uklotzde · 2023-02-20T21:20:49Z

src/widget/wsearchlineedit.cpp

@@ -803,3 +809,11 @@ void WSearchLineEdit::slotSetFont(const QFont& font) {
        updateClearAndDropdownButton(getSearchText());
    }
 }
+
+bool WSearchLineEdit::hasSelectedText() const {


uklotzde · 2023-02-20T21:21:48Z

src/widget/wsearchlineedit.cpp

@@ -517,6 +525,12 @@ void WSearchLineEdit::slotRestoreSearch(const QString& text) {
    enableSearch(text);
 }

+void WSearchLineEdit::triggerSearchDelayed() {


triggerSearchDebounced() for consistent naming?

uklotzde · 2023-02-20T21:24:29Z

src/widget/wsearchlineedit.cpp

-    case Qt::Key_Right:
+    case Qt::Key_Right: {
+        // Both keys may change or clear the selection (suggested completion).
+        const bool hadSelectedText = hasSelectedText();


hadSelectedTextBeforeKeyPressed for disambiguation of the very similar identifiers.

uklotzde · 2023-02-20T22:24:48Z

src/widget/wsearchlineedit.cpp

-        slotTriggerSearch();
+        // fetch lineEdit again, because we have no guarantee that pEdit
+        // is still valid after QComboBox::keyPressEvent()
+        if (hadSelectedTextBeforeKeyPressed && hasSelectedText()) {


Condition is wrong? Doesn't match the comment.

Good catch.

uklotzde

Please squash the commits.

ronso0 · 2023-02-21T02:14:01Z

Tbh, to me, this feels a bit complicated by now.

I thought of the QCompleter signals initially, thought it was too complicated, but I gave it a shot and it turns out to be pretty simple, and solid so far. Please take a look at 1bdd6eb. I'll open a PR tomorrow.

daschuer · 2023-02-21T06:39:33Z

My original approach was like this, but I did not manage to make it work reliable.
We need also consider that lineEdit is a subject of a change:
https://github.com/qt/qtbase/blob/e0a5915f88a780ba0791bce5ed042e92036a4900/src/widgets/widgets/qcombobox.cpp#L1743

Co-authored-by: ronso0 <ronso0@mixxx.org>

… removed. This fixes a regression described in issue mixxxdj#11287

daschuer · 2023-02-21T06:47:06Z

Rebase done.

daschuer · 2023-02-25T11:35:49Z

@ronso0: So we can merge this?

ronso0 · 2023-02-25T13:38:36Z

src/widget/wsearchlineedit.cpp

+        return false;
+    }
+    return pEdit->hasSelectedText();
+}


As I already pointed out earlier, this is obsolete:
There is always a QLineEdit because we set the combobox editable in the constructor, and we rely on that in many places here. Please see
https://github.com/qt/qtbase/blob/4903e45231b00fb8a06f0b3c60af764f13e831bd/src/widgets/widgets/qcombobox.cpp#L1787-L1802

Just use lineEdit() directly.

Also applies here 7165f0b#diff-85c225cd63139457cde84ddb0d2f69c854c16f8254672cd19587fd25dd755c62R316

The other changes LGTM, thanks!

daschuer · 2023-03-19T00:12:13Z

Done

daschuer · 2023-03-19T10:07:49Z

the pre-commit issue is caused by #11386

ronso0 · 2023-03-19T13:31:51Z

Thank you!

github-actions bot added the ui label Feb 20, 2023

daschuer mentioned this pull request Feb 20, 2023

Search bar: LibraryTableModel::select() invoked for each control key press #11287

Closed

daschuer added this to the 2.4.0 milestone Feb 20, 2023

uklotzde reviewed Feb 20, 2023

View reviewed changes

src/widget/wsearchlineedit.cpp Show resolved Hide resolved

uklotzde reviewed Feb 20, 2023

View reviewed changes

daschuer force-pushed the gh11287 branch from 73a0aea to dae4179 Compare February 20, 2023 22:49

uklotzde approved these changes Feb 21, 2023

View reviewed changes

daschuer and others added 3 commits February 21, 2023 07:41

Remove unused code in WSearchLineEdit

f08b4ea

Improve the description of WSearchLineEdit::getSearchText()

1aaa58a

Co-authored-by: ronso0 <ronso0@mixxx.org>

Only trigger a search via key left and right in case the selection is…

7165f0b

… removed. This fixes a regression described in issue mixxxdj#11287

daschuer force-pushed the gh11287 branch from dae4179 to 7165f0b Compare February 21, 2023 06:46

ronso0 mentioned this pull request Feb 22, 2023

Search: don't trigger search on each cursor keys press ... #11296

Closed

ronso0 requested changes Feb 25, 2023

View reviewed changes

Use lineEdit() directly like in other places. It cannot be null.

cb86a28

ronso0 approved these changes Mar 19, 2023

View reviewed changes

ronso0 merged commit e0401fc into mixxxdj:2.4 Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No longer trigger unneccessary searches om left and right cursor keys #11289

No longer trigger unneccessary searches om left and right cursor keys #11289

daschuer commented Feb 20, 2023

ronso0 commented Feb 20, 2023

ronso0 commented Feb 20, 2023

ronso0 commented Feb 20, 2023

uklotzde commented Feb 20, 2023

uklotzde Feb 20, 2023

daschuer Feb 20, 2023

ronso0 Feb 20, 2023

uklotzde Feb 20, 2023 •

edited

Loading

ronso0 Feb 20, 2023

daschuer commented Feb 20, 2023

uklotzde Feb 20, 2023

uklotzde Feb 20, 2023 •

edited

Loading

uklotzde Feb 20, 2023

uklotzde Feb 20, 2023 •

edited

Loading

daschuer Feb 20, 2023

uklotzde left a comment

ronso0 commented Feb 21, 2023

daschuer commented Feb 21, 2023

daschuer commented Feb 21, 2023

daschuer commented Feb 25, 2023

ronso0 Feb 25, 2023

ronso0 Feb 25, 2023

daschuer commented Mar 19, 2023

daschuer commented Mar 19, 2023

ronso0 commented Mar 19, 2023

No longer trigger unneccessary searches om left and right cursor keys #11289

No longer trigger unneccessary searches om left and right cursor keys #11289

Conversation

daschuer commented Feb 20, 2023

ronso0 commented Feb 20, 2023

ronso0 commented Feb 20, 2023

ronso0 commented Feb 20, 2023

uklotzde commented Feb 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Feb 20, 2023

Choose a reason for hiding this comment

uklotzde Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde left a comment

Choose a reason for hiding this comment

ronso0 commented Feb 21, 2023

daschuer commented Feb 21, 2023

daschuer commented Feb 21, 2023

daschuer commented Feb 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Mar 19, 2023

daschuer commented Mar 19, 2023

ronso0 commented Mar 19, 2023

uklotzde Feb 20, 2023 •

edited

Loading

uklotzde Feb 20, 2023 •

edited

Loading

uklotzde Feb 20, 2023 •

edited

Loading