Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Search: add special BPM filters #12072
Changes from 15 commits
9f87a54
77da3f8
b2490ea
6e588ec
c39003d
0542824
ffc5684
61d3708
1e30920
77b5391
57114ec
ac4b72b
9f1a2a2
382017d
0387942
865367d
50617dd
8ee131c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
So, ideally we'd have a regex using QLocale::groupSeparator() and QLocale::decimalPoint()
I'd prefer to merge this now to collect alpha/beat feedback and work on the regex in a follow-up.
@daschuer @ywwg Ready to roll?
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.
bpm is only used after this mode conditional branch, can you move it down to, say, line 600 or so? That way it's closer to where it's being used
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.
Well, we need to know if the argument is a double here already, and doing so with toDouble() gives us the bpm.
We may assign m_bpm = bpm right away at the top of the isDouble branch, regardless whether it'll be used or not (that would save the ::Operator case at the end of this function).
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.
this can be a debug assert -- in a release build we can just silently fail, but during development we'd want to know it was happening
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.
MatchMode::Invalid is not for programmming errors but for malformed typed queries (user error), for example
fuzzy range ~bpm:123-125
fuzzy operator ~bpm:<=95
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.
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.
Why it finds only a full match in the 1x case but a 1 BPM rang in other cases? I don't think that the underlying use case is different.
Did you consider to always match integer rounded values?
Or distinguish 123.0 for only 123.0 but 123 for 122.5 .. 123.5
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.
It's only a 1 BPM for half and double if the search term has decimals, because it that case it seems unlikely the user has/wants exact half/double matches.
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.
Yes I agree but why do the user want exact 1x matches but not exact 2x and 0.5x matched.
You can easily have the same track annotate as 123 or 62,5. So it will be found if it is in the 0.5x range but not in the 1x range. That doe not feel reasonable.
Did you consider to use this:
That means that the omitted decimal places are matching to the rounded value of the searched tracks.
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.
The source code comment is now outdated.