WSearchLineEdit: Add options to disable auto-completion and history #10942

fwcd · 2022-10-06T12:07:22Z

Fixes #10634

This adds two options (that are enabled by default) to configure whether search completions and history (including up/down keyboard shortcuts) should be enabled:

fwcd · 2022-10-06T12:38:37Z

Hm, the build failure is strange, is setAutoCompletion not available on all platforms?

daschuer · 2022-10-06T12:46:16Z

Can you explain your use case a bit more?
I assume that this is a solution for a usability issue and not an issue that you never want the feature...

fwcd · 2022-10-06T12:53:39Z

It's mostly minor things that I find a bit inconvenient:

The completions kicking in too quickly, as you've described in the issue
Accidentally hitting up/down or turning the track selection knob on the controller (while the search bar is focused) now moves through the history. While this of course is the intended behavior, I find it a bit irritating since I now have to seek back through the history and figure out what I previously wanted to type
I don't really use the feature, so being able to disable it makes the UI a bit less noisy

ronso0 · 2022-10-06T14:40:32Z

Just a side note, the new options should be in the Library page.

ronso0 · 2022-10-06T21:27:43Z

Figure out why setAutoCompletion doesn't compile on all platforms

Use setCompleter(completer*)?
Either completer* is a valid QCompleter or a nullptr (disables auto-completion as described in the doc).

Also, instead of parsing the config on each Up/Down/Space press, why not use a static functions to set bools, like the one setting the debounce timeout?
void WSearchLineEdit::setDebouncingTimeoutMillis(int debouncingTimeoutMillis)

Furthermore, I'm not comfortable with removing the history function altogether (which is not the case now btw, queries are still saved and restored, though not accessible).
Isn't it sufficient for your use case to suppress the emulated keystrokes sent by librarycontrol, i.e. still allow regular hotkeys (Up, Down, Ctrl+Space) and keep the expand button?

fwcd · 2022-10-07T14:05:34Z

I've moved the settings as per your suggestion to the Library page (see the updated screenshot in the PR description) and renamed the history setting to 'Enable search history shortcuts' to clarify that it only affects the shortcuts and not the history itself.

Suppressing the emulated keystrokes in LibraryControl would probably work but IMO feels a bit like the wrong place to model this (if we consider the search history to be an implementation detail of the search bar, we should also handle it there and not swallow the key events at a higher level). On the other hand, the generated keys are already somewhat tied to the search bar's semantics. Not sure about this one.

Regarding the static functions: While this may work for the history shortcuts (since we only need to read the value during key events), setSearchCompletionsEnabled cannot really be static if we want to to swap the completer based on whether search completions are enabled. The todo comment mentions that this also is more of a workaround currently and that it would be better to wire up the config changes with signals and slots. Since we would have access to the WSearchLineEdit instance that way, this would also solve the aforementioned issue.

Since I'm not that familiar with the data flow of configuration changes yet, do you have some pointers on how to do cleanly propagate config changes to WSearchLineEdit?

ronso0 · 2022-10-08T10:49:55Z

setSearchCompletionsEnabled cannot really be static if we want to to swap the completer based on whether search completions are enabled.

You could check if this bool changed in the focusInEvent()

ronso0 · 2022-10-08T10:51:15Z

Anyways, I assume this setting is changed rarely, so applying this after start / skin change/reload may be sufficient IMO

As per the PR suggestions: - mixxxdj#10942 (comment) - mixxxdj#10942 (comment)

fwcd · 2022-10-11T17:36:14Z

I've now wired up the settings as static bools as you suggested. Also, WSearchLineEdit now creates a custom owned QCompleter that is set depending on whether search completions are enabled.

One thing that struck me as slightly odd is that some settings are updated differently: For example, the debounce time is updated immediately when the user changes the widget whereas most other settings are first applied when the user clicks 'Apply'. This feels a bit inconsistent (but is probably a topic for another PR). With these new settings, I've tried to match the original semantics (i.e. first update the config when 'Apply' is clicked).

In the longer term, we'd probably want to find a clean way to notify WSearchLineEdit about config changes, the updating-static-variables feels like a workaround to a deeper architectural issue currently.

ywwg · 2022-10-22T15:28:49Z

I support this feature, I do not personally like the autocomplete / history but I understand others may find it useful.

ronso0

I'm sorry you had to wait so long, this was kinda pushed off the table.

It's working nicely, thanks. Just some nitpicks.
I guess it doesn't matter that the completer is always created even if it will never actually be loaded to lineEdit->completer.

src/preferences/dialog/dlgprefinterface.cpp

src/widget/wsearchlineedit.cpp

src/widget/wsearchlineedit.h

src/preferences/dialog/dlgpreflibrarydlg.ui

src/widget/wsearchlineedit.h

ronso0 · 2022-11-22T00:05:52Z

I think it works also ~~with~~ without the "Search history:" label (also we need less space and thereby potentially avoid horizontal scrollbars for long translations)

Enable search history keyboard shortcuts ?

As per the PR suggestions: - mixxxdj#10942 (comment) - mixxxdj#10942 (comment)

fwcd · 2022-11-25T17:52:54Z

I've updated the branch with your suggestions

ronso0 · 2022-11-27T12:19:41Z

src/widget/wsearchlineedit.cpp

 #include <QAbstractItemView>
 #include <QApplication>
+#include <QCompleter>


Oh, during my previous review I didn't notice this is already included in wsearchlineedit.h
Please remove it here, git commit --amend (maybe name it more explicitely) and push one last time.

FYI rebasing after a review without approval is not appreciated since that means someone has to review again, instead of just checking the last commits. Here, it's not much of a drama because the change set is rather small and can be verified by manual testing.

FYI rebasing after a review without approval is not appreciated

Ah sorry, I'll try to keep that in mind for the next time (in this case the rebase was useful since the row numbering in dlgpreflibraryui changed upstream and would have silently merged without a conflict)

I didn't neither review again nor test manually, so if you like to --fixup previous commits with the latest changes and squash.. feel free.

I went with amending for now, most of these are just small stylistic changes anyway

fwcd · 2023-01-04T20:25:41Z

Any updates on this?

daschuer · 2023-01-18T09:30:36Z

After #11207 is merged, the completion is no longer applied immediately. It requires "→" or "⏎" to take effect.

This means the

The completions kicking in too quickly, as you've described in the issue

part will be solved.

Can we reduce this to only add the "EnableSearchHistoryShortcuts" preferences option for less clutter in the preferences?

fwcd · 2023-01-18T09:36:22Z

I still think having the option to disable auto-completion would be cool, if only to reduce visual noise

daschuer · 2023-01-18T09:33:34Z

src/widget/wsearchlineedit.h

+    static constexpr bool kEnableCompletionsByDefault = true;
+    static constexpr bool kEnableHistoryShortcutsByDefault = true;


This is a function name, how about

Suggested change

static constexpr bool kEnableCompletionsByDefault = true;

static constexpr bool kEnableHistoryShortcutsByDefault = true;

static constexpr bool kDefaultSearchCompletionsEnabled = true;

static constexpr bool kDefaultSearchHistoryShortcutsEnabled = true;

I preferred to remove the redundant 'search' string, so let's name these
kCompletionsEnabledDefault
kHistoryShortcutsEnabledDefault
?

daschuer · 2023-01-18T09:41:59Z

src/preferences/dialog/dlgpreflibrary.cpp

@@ -495,6 +513,15 @@ void DlgPrefLibrary::slotSearchDebouncingTimeoutMillisChanged(int searchDebounci
    WSearchLineEdit::setDebouncingTimeoutMillis(searchDebouncingTimeoutMillis);
 }

+void DlgPrefLibrary::searchHistoryOptionsChanged() {


Since this is not a slot, name the function according to what it does, not when it is called.

Suggested change

void DlgPrefLibrary::searchHistoryOptionsChanged() {

void DlgPrefLibrary::updateSearchLineEditHistoryOptions() {

daschuer · 2023-01-18T10:02:08Z

I still think having the option to disable auto-completion would be cool, if only to reduce visual noise

OK

daschuer

LGTM Thank you.

github-actions bot added the ui label Oct 6, 2022

fwcd force-pushed the configurable-search-bar branch from 78c3c9c to 8328736 Compare October 6, 2022 12:10

ronso0 changed the title ~~Interface: Add options to disable search completions and history~~ WSearchLineEdit: Add options to disable auto-completion and history Oct 6, 2022

fwcd added a commit to fwcd/mixxx that referenced this pull request Oct 11, 2022

WSearchLineEdit: Model search history options as statics

c549daf

As per the PR suggestions: - mixxxdj#10942 (comment) - mixxxdj#10942 (comment)

fwcd force-pushed the configurable-search-bar branch from 60b99fe to c549daf Compare October 11, 2022 17:13

github-actions bot added the library label Oct 11, 2022

fwcd marked this pull request as ready for review October 11, 2022 17:31

ronso0 requested changes Nov 21, 2022

View reviewed changes

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

fwcd added 9 commits November 24, 2022 23:31

DlgPrefInterface: Add checkboxes for configuring search bar

007817f

DlgPrefInterface: Wire up search bar options

b51a787

WSearchLineEdit: Respect history enabled setting

d01584f

WSearchLineEdit: Respect search completion option

12b68a3

DlgPrefLibrary: Move search history options here

256e232

Library: Rename option to EnableSearchHistoryShortcuts

2d6bb97

WSearchLineEdit: Use setCompleter to set completions

a2c892f

WSearchLineEdit: Model search history options as statics

0db64de

As per the PR suggestions: - mixxxdj#10942 (comment) - mixxxdj#10942 (comment)

WSearchLineEdit: Use custom QCompleter

d1a78df

fwcd force-pushed the configurable-search-bar branch from ba3a787 to a10dc31 Compare November 25, 2022 17:51

fwcd force-pushed the configurable-search-bar branch from a10dc31 to 6e5d3be Compare November 25, 2022 17:55

ronso0 reviewed Nov 27, 2022

View reviewed changes

WSearchLineEdit: Apply PR suggestions

5340f35

fwcd force-pushed the configurable-search-bar branch from 6e5d3be to 5340f35 Compare November 27, 2022 14:40

ronso0 added this to the 2.4.0 milestone Dec 4, 2022

ronso0 mentioned this pull request Jan 18, 2023

Don't immediately search for auto completed strings. #11207

Merged

ronso0 approved these changes Jan 18, 2023

View reviewed changes

daschuer requested changes Jan 18, 2023

View reviewed changes

WSearchLineEdit: Address PR suggestions

65375b2

daschuer changed the base branch from main to 2.4 January 18, 2023 11:08

daschuer approved these changes Jan 18, 2023

View reviewed changes

daschuer merged commit 89f5860 into mixxxdj:2.4 Jan 18, 2023

fwcd deleted the configurable-search-bar branch January 18, 2023 12:41

ronso0 mentioned this pull request Mar 24, 2023

Add option to disable search completions #10634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WSearchLineEdit: Add options to disable auto-completion and history #10942

WSearchLineEdit: Add options to disable auto-completion and history #10942

fwcd commented Oct 6, 2022 •

edited

Loading

fwcd commented Oct 6, 2022

daschuer commented Oct 6, 2022

fwcd commented Oct 6, 2022 •

edited

Loading

ronso0 commented Oct 6, 2022

ronso0 commented Oct 6, 2022

fwcd commented Oct 7, 2022

ronso0 commented Oct 8, 2022

ronso0 commented Oct 8, 2022

fwcd commented Oct 11, 2022

ywwg commented Oct 22, 2022

ronso0 left a comment

ronso0 commented Nov 22, 2022 •

edited

Loading

fwcd commented Nov 25, 2022

ronso0 Nov 27, 2022

fwcd Nov 27, 2022 •

edited

Loading

ronso0 Nov 27, 2022

fwcd Nov 27, 2022

fwcd commented Jan 4, 2023

daschuer commented Jan 18, 2023

fwcd commented Jan 18, 2023

daschuer Jan 18, 2023

ronso0 Jan 18, 2023

daschuer Jan 18, 2023

daschuer commented Jan 18, 2023

daschuer left a comment

		static constexpr bool kEnableCompletionsByDefault = true;
		static constexpr bool kEnableHistoryShortcutsByDefault = true;

	void DlgPrefLibrary::searchHistoryOptionsChanged() {
	void DlgPrefLibrary::updateSearchLineEditHistoryOptions() {

WSearchLineEdit: Add options to disable auto-completion and history #10942

WSearchLineEdit: Add options to disable auto-completion and history #10942

Conversation

fwcd commented Oct 6, 2022 • edited Loading

Fixes #10634

fwcd commented Oct 6, 2022

daschuer commented Oct 6, 2022

fwcd commented Oct 6, 2022 • edited Loading

ronso0 commented Oct 6, 2022

ronso0 commented Oct 6, 2022

fwcd commented Oct 7, 2022

ronso0 commented Oct 8, 2022

ronso0 commented Oct 8, 2022

fwcd commented Oct 11, 2022

ywwg commented Oct 22, 2022

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 commented Nov 22, 2022 • edited Loading

fwcd commented Nov 25, 2022

ronso0 Nov 27, 2022

Choose a reason for hiding this comment

fwcd Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

ronso0 Nov 27, 2022

Choose a reason for hiding this comment

fwcd Nov 27, 2022

Choose a reason for hiding this comment

fwcd commented Jan 4, 2023

daschuer commented Jan 18, 2023

fwcd commented Jan 18, 2023

daschuer Jan 18, 2023

Choose a reason for hiding this comment

ronso0 Jan 18, 2023

Choose a reason for hiding this comment

daschuer Jan 18, 2023

Choose a reason for hiding this comment

daschuer commented Jan 18, 2023

daschuer left a comment

Choose a reason for hiding this comment

fwcd commented Oct 6, 2022 •

edited

Loading

fwcd commented Oct 6, 2022 •

edited

Loading

ronso0 commented Nov 22, 2022 •

edited

Loading

fwcd Nov 27, 2022 •

edited

Loading