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

Searchbar clear #3262

Merged
merged 11 commits into from
Feb 17, 2021
Merged

Searchbar clear #3262

merged 11 commits into from
Feb 17, 2021

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Nov 3, 2020

Clear the searchbar on up-key after the first entry and remove
tab focus from searchbar clear button.

This way, the tab order is fixed again while keeping the possibility
to use the arrow mapping for clearing the search bar.
@github-actions github-actions bot added the ui label Nov 3, 2020
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM and works nicely. Thank you.

src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved
@@ -97,6 +97,7 @@ WSearchLineEdit::WSearchLineEdit(QWidget* pParent)

m_clearButton->setCursor(Qt::ArrowCursor);
m_clearButton->setObjectName(QStringLiteral("SearchClearButton"));
m_clearButton->setFocusPolicy(Qt::ClickFocus);
Copy link
Member

@ronso0 ronso0 Nov 4, 2020

Choose a reason for hiding this comment

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

I don't agree:
how do you quickly clear the search from a controller now?
Scrolling all the way to the top of the search history is no option for me.

I know this was requested on Zulip because someone uses a custom mappin for the Trax encoder press and wants to reduce the Tabs necessary to get to the tracks table, but the default most common mapping is [Library],GoToItem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current system is also problematic for blind users as the tab order changes depending on the state of the search bar.

I'm ambivalent about the focus behaviour. What's the general stand towards this ?

@ronso0 ronso0 added this to In progress in 2.4 release via automation Nov 16, 2020
@ronso0 ronso0 modified the milestones: 2.3.0, 2.4.0 Nov 16, 2020
@poelzi
Copy link
Contributor Author

poelzi commented Jan 18, 2021

Since there was not much feedback and I don't care that much, I reverted the focus change and let the keyup clearing stay. Ready for merge.

@daschuer
Copy link
Member

I have tested the behavior again and found that the whole history is cleared with the clear icon:
https://bugs.launchpad.net/mixxx/+bug/1913660
That is at least surprising.

Do we need the clear history feature? If yes, we may add a dummy entry at the very end.

This can be merged as it is IMHO.

@rons0: What do you think?

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2021

Well, it works when applied to a search that is present already, index = 0
Else the index remains -1 until the savetimer expired (5000 ms) or Down is pressed (even if it's the only entry atm).

We should stop the saveTimer and clear the field in that case.
if (findCurrentTextIndex() == -1 && !currentText().isEmpty()) { m_saveTimer.stop(); ...
It would be consistent to save the term if the debouncing timer already expired (default is 300ms), but maybe we accumulate to much unused items in the combobox -- which might be an issue when scrolling through the search index list, until we have means to remove individual indices from the list.

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2021

please check poelzi#4
it fixes the save timer issue, and also lp:1913660 Search bar clear button clears entire search history

@ronso0
Copy link
Member

ronso0 commented Feb 14, 2021

btw ping @poelzi

@ronso0
Copy link
Member

ronso0 commented Feb 15, 2021

actually ready to be merged, but now there's a small issue with the popup:

  • populate the list with 1, 2, 3 top>bottom (press 3, Enter, Up, press 2, Enter, Up, 1 ...)
  • clear the search (Up or X)
  • hit Ctrl + space to open the popup
  • press Down = nothing happens, nothing selected
  • press Down = 2 selected
  • press Down = 3 selected
  • press Down = 3 selected
  • press Up = 3 selected

@ronso0
Copy link
Member

ronso0 commented Feb 16, 2021

could fix it by only filtering for Up/Down when the popup is closed, but I don't wanna highjack your PR

@ronso0
Copy link
Member

ronso0 commented Feb 17, 2021

poelzi#5
fixes the selection issue, and uses official lineEdit()->clear(). debouncing is still applied.

@ronso0 ronso0 merged commit d6af08b into mixxxdj:main Feb 17, 2021
2.4 release automation moved this from In progress to Done Feb 17, 2021
@ronso0
Copy link
Member

ronso0 commented Feb 17, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
2.4 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants