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

Search related Tracks: add checkboxes + Search button #12213

Merged
merged 5 commits into from
May 29, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Oct 24, 2023

Closes #12211
Implementation adopted from Crates menu.

Menu items are checkboxes now, so there are two ways to search for tracks:

  • single filter (as before):
    click an action (text), or press Return on a selected checkbox, to trigger its search
  • combine filters:
    • check multiple actions (click [ ] box or press Space on a selected item
    • Search selected is enabled if at least one box is checked
    • click Search selected (or press S, or Return on the selected action) to search with all checked filters

image

While this may actually go into 2.4 I'm targeting main because this adds yet another tr string ("Search selected").

@github-actions github-actions bot added the ui label Oct 24, 2023
@ronso0
Copy link
Member Author

ronso0 commented Oct 24, 2023

@uklotzde What do you think about this?

@ronso0
Copy link
Member Author

ronso0 commented Oct 24, 2023

Nice to have: remember & restore checked state of filters.

This would simplify repeating the previous filter combo, e.g. searching again for genre and BPM then only requires pressing Search (S). For good UX (in decks) the check state should be shared by all track menu widgets of a certain deck.

Either have only one track menu per deck (track menu features are/should be the same anyway for WTrackProperty, WTrackText & WTrackWidgetGroup). I think LegacySkinParser could take care of this, e.g. with a QHash<QString group, WTrackMenu* menu>, though this probably requires reverting #12212.

Alternatively, keep the per-widget menu and share just the checked flags elsewhere.

@ronso0
Copy link
Member Author

ronso0 commented Oct 25, 2023

Now clazy is inhibited by clang-tidy failing on unrelated files 🤷

@uklotzde
Copy link
Contributor

uklotzde commented Oct 26, 2023

I only consider key, bpm, and genre as useful conjunctions. Everything else is probably too restrictive in the context of DJing. Even combining the genre filter with others is too restrictive if you encode multiple genres in the field as I do.

Remembering and recalling the last checkmarks is not helpful. I often switch between different filters, e.g. artist and directory/album. That would require lots of extra clicks.

Clicking at least twice for selecting a filter before actually starting the search is also inconvenient. Resolved.

To me this extension seems more like a workaround for the absence of dynamic filters and smart playlists. Using the properties of the selected track instead of hard-coded values for the filter conditions of a smart playlist would be a new concept.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2023

Clicking at least twice for selecting a filter before actually starting the search is also inconvenient.

Why twice?

  • single filter (as before):
    * click an action (text), or press Return on a selected checkbox, to trigger its search

  • combine filters:
    * check multiple actions (click [ ] box or press Space on a selected item
    * click Search (or press S, or Return on Search) to search with all checked filters

@uklotzde
Copy link
Contributor

If no additional clicks are required for searching with a single condition then this extension is reasonable.

@uklotzde
Copy link
Contributor

The last item should be renamed to "Search checked" and only be enabled if one or more check boxes are checked.

@ronso0
Copy link
Member Author

ronso0 commented Oct 26, 2023

only be enabled if one or more check boxes are checked.

That's how it's implemented, disabled by default.

"Search checked" is indeed better, thanks.

edit
It's now "Search selected".

@ronso0 ronso0 force-pushed the search-related-checkboxes branch 2 times, most recently from 6ac5739 to 3ed07df Compare October 28, 2023 21:04
@ronso0 ronso0 marked this pull request as ready for review October 29, 2023 08:57
Menu items are checkboxes now, so there are two ways to search for tracks:
* single filter (as before):
  click an action (text), or press `Return` on a selected checkbox, to trigger its search
* combine filters:
   * check multiple actions (click `[ ]` box or press `Space` on a selected item
   * click 'Search selected' (or press `S`, or `Return` on Search) to search with all checked filters
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 30, 2024
@ronso0 ronso0 added this to the 2.5-beta milestone May 16, 2024
@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label May 18, 2024
@daschuer
Copy link
Member

This works good. However with Qt5 I don't see the magnifier icon.
The workflow is usable, but it is actually a pop up window with a button disguised as a menu.

Did you consider to give the "Search selected" menu entry a button style.
This would allow to "test" checkboxes without to select them again after no result and reopening then menu.

Now building with Qt6

@daschuer
Copy link
Member

Also no magnifier icon with Qt6.
This is no blocker though.

@ronso0
Copy link
Member Author

ronso0 commented May 22, 2024

The workflow is usable, but it is actually a pop up window with a button disguised as a menu.

Indeed. Thinking this further, the ideal solution would be a 'Filter' popup attached to the searchbar with customizable rows
[tag selector] [contains|equal|not] [text]
with 'Pin search' and other features we already discussed.
In case of "Search related.." that would be populated with the selected checkboxes/filters.

@ronso0
Copy link
Member Author

ronso0 commented May 22, 2024

Did you consider to give the "Search selected" menu entry a button style.

I'm not sure if that really makes the situation more clear.

@ronso0
Copy link
Member Author

ronso0 commented May 22, 2024

This would allow to "test" checkboxes without to select them again after no result and reopening then menu.

Which use case do you intend to cover?
Either the selected filters yields valuable results or not, and if not users may refine the query manually.

@ronso0
Copy link
Member Author

ronso0 commented May 22, 2024

Also no magnifier icon with Qt6.

Oh, the screenshot is from a previous state.
I'll try to restore it.

Copy link
Member

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

Just a small nit from clang-tidy

Couple of feedback on UI - looks like the checkbox doesn't render when selected. Also, it be great if the menu would stay open till it loose the focus after the first click, so you can easily select key + BPM for example. (edit: realise I had to click the checkbox. Still would be great, but nice to have only
Edit 2: Realised that the Search related at the bottom is meant to be a submit button. Make sense now)

image

Note that I merged main as I needed TagLib 2.0 support, maybe why these issue didn't occur before?

Love the feature btw! So many time I struggle to filter key + bpm and end up having to cheat with sorting... My last wish would be a configurable kRelativeBpmRange or the the ability to define the range :D

}
QStyleOptionButton option;
option.initFrom(box);
auto pStyle = box->style();
Copy link
Member

Choose a reason for hiding this comment

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

clang-tidy

Suggested change
auto pStyle = box->style();
auto *pStyle = box->style();

@acolombier
Copy link
Member

A bit of an issue with controller compatibility:
I often search related track using the controllers ([Library],focused_widget/[Library],GoToItem), it'd bee nice if that could be compatible with that. I guess we need to make GoToItem trigger the checkbox rather than the action?

@ronso0
Copy link
Member Author

ronso0 commented May 24, 2024

@acolombier Thanks for testing!

So many time I struggle to filter key + bpm and end up having to cheat with sorting... My last wish would be a configurable kRelativeBpmRange or the the ability to define the range :D

In main you can set that range in Pref -> Library. It's used for the fuzzy BPM search and for the "Search related" menu.
I'll merge main soonish.

@ronso0
Copy link
Member Author

ronso0 commented May 24, 2024

A bit of an issue with controller compatibility: I often search related track using the controllers ([Library],focused_widget/[Library],GoToItem), it'd bee nice if that could be compatible with that. I guess we need to make GoToItem trigger the checkbox rather than the action?

I understand. You may add an extra FocusWidget type, e.g. SearchMenu, here and use Qt::Key_space for that here.
Or, depending on what else you use GoToItem for, simply always use Space everywhere.

@github-actions github-actions bot added the skins label May 25, 2024
@acolombier
Copy link
Member

acolombier commented May 25, 2024

You may add an extra FocusWidget type, e.g. SearchMenu, here and use Qt::Key_space for that here.

Sound good. I might give that a go once its merged so that feature is available mouse/click-free.

In main you can set that range in Pref -> Library. It's used for the fuzzy BPM search and for the "Search related" menu.

Humm looks it might not work.. I'll create an issue for that. Works just fine!


Tested and search button now works fine on Qt6!

@ronso0
Copy link
Member Author

ronso0 commented May 25, 2024

You may add an extra FocusWidget type, e.g. SearchMenu, here and use Qt::Key_space for that here.

Sound good. I might give that a go one its merged so that feature is available mouse/click-free.

I just implemented it 😆
Will open a separate PR for it.

@ronso0
Copy link
Member Author

ronso0 commented May 25, 2024

Humm looks it might not work.. I'll create an issue for that Works just fine!

Can you try to find out why it didn't work? Maybe with a clean config?

@acolombier
Copy link
Member

No, it's was just my eyes that were failing me 😅
Changing the setting was leading the criteria to move from 98~102 to 92~108 which for some reason I thought was the same value - that 2/8 swap tricked me!

@daschuer
Copy link
Member

This would allow to "test" checkboxes without to select them again after no result and reopening then menu.

Which use case do you intend to cover?
Either the selected filters yields valuable results or not, and if not users may refine the query manually.

It is just the minor issue that the "refine" is more like a "redo", because after selecting "search related" the menu is closed and you need to repopulate the check boxes before manually on a second attempt.

(Just an idea ...)

@ronso0
Copy link
Member Author

ronso0 commented May 26, 2024

Yes, the clearing can be a bit annoying, I already added a TODO inline
https://github.com/mixxxdj/mixxx/pull/12213/files#diff-848ff12a6bbb8c39f28e2a6be33ca2bbbe31f53e6ba550b2f6df7b8bdfaecf1fR227-R230

Saving the check states is a bit more eleaborate, at least if you refer to when opening the entire track menu again.

@ronso0
Copy link
Member Author

ronso0 commented May 26, 2024

There is/was a slight delay after clicking search actions or the Search button (also a checkbox internally), I have the impression it's longer with this PR. Can someone confirm?

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, @acolombier is your issue also solved?

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated?

}
QStyleOptionButton option;
option.initFrom(box);
auto pStyle = box->style();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto pStyle = box->style();
auto* pStyle = box->style();

if (queries.isEmpty()) {
return;
} else {
QString queryCombo = queries.join(QStringLiteral(" "));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
QString queryCombo = queries.join(QStringLiteral(" "));
QString queryCombo = queries.join(QChar(' '));

return;
}
m_pSearchAction->setDisabled(true);
for (const auto* child : std::as_const(children())) {
Copy link
Member

Choose a reason for hiding this comment

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

is already const see: https://doc.qt.io/qt-6/qobject.html#children

Suggested change
for (const auto* child : std::as_const(children())) {
for (const auto* child : children()) {

void WSearchRelatedTracksMenu::combineQueriesTriggerSearch() {
// collect queries of all checked checkboxes
QStringList queries;
for (const auto* child : std::as_const(children())) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const auto* child : std::as_const(children())) {
for (const auto* child : children()) {

Copy link
Member

Choose a reason for hiding this comment

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

You may also decorate child and friends with a "p" prefix

@ronso0 ronso0 force-pushed the search-related-checkboxes branch from 6679db8 to e897d66 Compare May 26, 2024 21:12
@daschuer daschuer changed the base branch from main to 2.5 May 26, 2024 21:50
@acolombier
Copy link
Member

@acolombier is your issue also solved

Yeah, the only issue I had was sitting between the chair and the keyboard 😅

Only thing is I think it would be great to keep state between context menu, but that can come as an iteration.

@daschuer
Copy link
Member

Fine. Thank you.

@daschuer daschuer merged commit a53437e into mixxxdj:2.5 May 29, 2024
13 checks passed
@ronso0 ronso0 deleted the search-related-checkboxes branch May 29, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search related Tracks: allow to combine filters
4 participants