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.

Sign up for GitHub

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

Jump to bottom

Search bar: improve keypress handling, fix glitch in popup, strip whitespaces #4658

Merged
merged 9 commits into from
May 6, 2022
Merged

Search bar: improve keypress handling, fix glitch in popup, strip whitespaces #4658

merged 9 commits into from
May 6, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Feb 2, 2022

This fixes some minor, yet annoying, UX issues.

@ronso0 ronso0 added this to the 2.4.0 milestone Feb 2, 2022
@github-actions github-actions bot added the ui label Feb 2, 2022
@ywwg
Copy link
Member

ywwg commented Feb 3, 2022

Can you explain how I can test this change to see what's different? I'm not quite sure what popup is being talked about

@ronso0
Copy link
Member Author

ronso0 commented Feb 3, 2022 •
edited
Loading

It's about the query popup > Ctrl+Space, or click the down arrow on the right.
Edit: you need saved queries to get the popup ;)

These are small commits with clear messages, so I hope the details are clear.

@ywwg
Copy link
Member

ywwg commented Feb 5, 2022

I get it! taking a look now

ywwg
ywwg approved these changes Feb 5, 2022
View reviewed changes
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

Works as described, lgtm. feel free to merge

@ronso0
Copy link
Member Author

ronso0 commented Feb 6, 2022

I pushed two fixup commits that simplify things a bit.
I'll squash those and rebase when I either get an okay or if a few days pass without another review.

Also I need to check why 46103f1 works in LateNight -- but has no effect in the other skins : |
(slotClearSearch() while the popup is open would still resize the searchbox in those)

@Holzhaus Holzhaus marked this pull request as draft February 6, 2022 01:30
@Holzhaus
Copy link
Member

Holzhaus commented Feb 6, 2022

Converting to draft to prevent merging these fixup commits.

uklotzde
uklotzde suggested changes Feb 6, 2022
View reviewed changes
src/widget/wsearchlineedit.cpp Outdated
@@ -233,7 +225,7 @@ void WSearchLineEdit::loadQueriesFromConfig() {
m_pConfig->getKeysWithGroup(kSavedQueriesConfigGroup);
QSet<QString> queryStrings;
for (const auto& queryKey : queryKeys) {
const auto& queryString = m_pConfig->getValueString(queryKey);
const auto& queryString = m_pConfig->getValueString(queryKey).simplified();
Copy link
Contributor

@uklotzde uklotzde Feb 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A query may intentionally contain multiple spaces and they might be quoted. They are not redundant and we shouldmust not remove them. Only leading/trailing spaces are redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

now uses .trim()

src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved
src/widget/wsearchlineedit.cpp Outdated
@@ -465,27 +472,32 @@ void WSearchLineEdit::slotTriggerSearch() {

/// saves the current query as selection
void WSearchLineEdit::slotSaveSearch() {
m_saveTimer.stop();
QString cText = currentText();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should apply trim() to avoid storing empty searches (see below, the check for `isEmpty())

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved
@ronso0 ronso0 force-pushed the search-key-popup-tweaks branch from 000667b to 2b21769 Compare February 17, 2022 11:43
@ronso0 ronso0 marked this pull request as ready for review February 17, 2022 11:43
uklotzde
uklotzde suggested changes Feb 20, 2022
View reviewed changes
src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved
src/widget/wsearchlineedit.cpp Outdated Show resolved Hide resolved
src/widget/wsearchlineedit.cpp Show resolved Hide resolved
@ronso0 ronso0 force-pushed the search-key-popup-tweaks branch from db9d1bd to 57938ea Compare March 22, 2022 19:11
@ronso0
Copy link
Member Author

ronso0 commented Apr 23, 2022

@uklotzde What's your opinion on the current state?

uklotzde
uklotzde approved these changes May 6, 2022
View reviewed changes
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

No more objections. Thanks for leaving comments that explain why some special case handling is required. LGTM

@uklotzde uklotzde merged commit c1a02ca into mixxxdj:main May 6, 2022
@ronso0 ronso0 deleted the search-key-popup-tweaks branch May 6, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

@ywwg ywwg ywwg approved these changes

Assignees
No one assigned
Labels
minor bug ui
Projects
2.4 release
Status: Done
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ronso0 @ywwg @Holzhaus @uklotzde