-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: add special BPM filters #12072
Conversation
29fcc79
to
8ecff43
Compare
fcd122a
to
685b1a2
Compare
This works as intended now. We may discuss the default behaviour and the fuzzy options:
|
685b1a2
to
bb71d01
Compare
cd3be83
to
37e85a4
Compare
This works beautifully now, please test. Manual PR coming soon. edit |
feb0518
to
b2d1f25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only looked at the first commit so far. The rest got too complicated too quickly for me. I think that we can merge without N/2 & N*2 as well as the configurable config option since those two changes incur the most complexity.
Half/was the reason I started this. Tbh I'm not motivated to split that up, first I'll wait for a real Mixxx session and test it in real life. |
I'm sorry, I'm not able to review this further then right now. |
Never mind, didn't expect anyone soon. Thanks for taking a look. |
b2d1f25
to
4850178
Compare
I've split this up into a bit, and I removed the half/double config option since that can be overwritten easily with the |
e74b713
to
db5357b
Compare
Done. New DEBUG commit is ec9b557 |
ddb6d30
to
be6f2b2
Compare
be6f2b2
to
81be9dd
Compare
81be9dd
to
0387942
Compare
src/test/searchqueryparsertest.cpp
Outdated
|
||
EXPECT_STREQ( | ||
qPrintable(QString("(bpm >= 126.95 AND bpm < 128) OR " | ||
"(bpm BETWEEN 63 AND 64) OR " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this test, I think I got your point:
If we consider that we have a electronic music library with only *.0 BPMs the resulting 1/2 range of >= 63,5 and < 64.0
64 will produce no match. Even if matches they are only 1% off.
Is this your rational? Can you put that somewhere as comment?
src/test/searchqueryparsertest.cpp
Outdated
EXPECT_STREQ( | ||
qPrintable(QString("(bpm >= 126.95 AND bpm < 128) OR " | ||
"(bpm BETWEEN 63 AND 64) OR " | ||
"(bpm BETWEEN 253 AND 255)")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
127 * 2 = 254
128 * 2 = 256
following the rational above we will find *.0 tracks with just bpm >= 254.95 AND bpm < 255
The current implementation lead to a not fully overlapping range:
original
127 [----[ 128
x 2 (my expectation)
254 [--------[ 256
implementation
253 [--------] 255
/2
126,5 [----] 127,5
Do we have a reason for this to match tracks, that would not be no longer match after editing the BPM /2?
I have probably not understand your goal.
The scenario I worked with is that I have a 127 BPM track playing and I want tracks that match somehow (doubel / half) without having to think about rounding or tempo correction in advance. I didn't think of doing /2 queries manually and thus didn't have the expectation that searching for BPM any of the found tracks would reveal certain tracks from the previous search. I tried to keep the range setups simple and would love to get this merged soonish if the code looks good, then get feedback and refine certain match modes. I need to play with the current implementation myself, too. |
@daschuer Is there specific behavior in the parser that you consider blocking your approval of this PR? Can you be specific about which query results you consider incorrect? |
OK, I will give it a try. |
You will find my proposal here: ronso0#67 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ready for merge after CI is done?
Thank you very much for this nice enhancement!
Yup, IMO this is ready to go. I'll update the manual ASAP. |
After a final test I have discovered on remaining issue, that an even bpm value only reveals the single exact halve value. I have fixed it here: |
|
||
// Replace the locale's decimal separator with . | ||
// This is handy if numbers are typed with the numpad. | ||
argument.replace(',', '.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should simply use QLocale().toDouble() below.
This might fail for 123.456,00
in case users have mixed locales (see #13051).
Currently it would fail if users type gb/us thousands separators 1,230.5
Both is acceptable IMO ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered your solution as quite smart, because we display the bpm with dot separator in the GUI and Library table but use a comma separator in editors. It is unlikely that one is using bpms above 999.
This issue is however only present in case the user has a locale with comma separators. In case of a dot separator local everything is consistent and the hack is probably not that smart. We should make it conditional then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the hold-up. lgtm! |
…kBox_enable_search_history_shortcuts Fixes the copy-paste error introduced during a refactoring in commit 6e588ec (merged with PR mixxxdj#12072).
rework of #12070
Closes #8191
This adds
BpmFilterNode
which allows the following special filtering:~bpm:N
findsN
+- n%n is % of the current rate slider range, can be configured in Pref > Library, default is 75%
bpm:N
finds N¹ as well as N/2 and N*2 (in case N is odd N/2 is replaced with a range of prev - next integer, considering electronic music with mostly integer BPM)can be toggled in Pref > Library, default ON
¹N is turned into a range to cover the rounded values (in the tracks table or BPM widgets).
For example 102.5 becomes [102.45 - 102.55]
Btw, this works beautifully with the new
OR
operator:~bpm:80 | ~bpm:160
: )Results:
~bpm:100
: 94-106 NEWbpm:125.2
: [125.15 - 125.25] and [250 - 251] and [52 - 53] NEW-bpm:100
: anything except exactly 100.0bpm:=100
: 125.15 - 125.25Preferences > Library
I re-grouped the options a bit, though we may discuss details like Metadata /Playlist title and the search note label
TODO
bpm
=
operator (we just need to document it)Manual