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

Round BPM display value in library table #2001

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

benis
Copy link
Contributor

@benis benis commented Jan 20, 2019

This was discussed a while ago on Zulip. This PR rounds the BPM column in the library to 1dp. The tooltip will still show the value to 3dp, and the actual value is obviously unaffected.

Below is pre-edit comment where values were rounded to whole numbers - ignore:

Before:
screenshot 2019-01-20 at 16 53 30

After:
screenshot 2019-01-20 at 16 53 59

The only snag I can think of is that users unfamiliar with Mixxx might be caught out by the fact that Mixxx's BPM detection doesn't always correctly identify tracks that are at whole BPMs. This might mask some cases where the analyzed BPM value isn't quite right, which could have some knock-on effects for bpm sync.

@benis benis changed the title Round display value in library table to whole number Round BPM display value in library table to whole number Jan 20, 2019
@benis benis changed the title Round BPM display value in library table to whole number Round BPM display value in library table Jan 20, 2019
@daschuer
Copy link
Member

Which issue does the PR solve?

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.

I don't think that this will work for all users.
Is it worth to clutter the preferences with this?

@benis
Copy link
Contributor Author

benis commented Jan 20, 2019

Which issue does the PR solve?

I was thinking of this comment in particular which a number of people agreed with. I realise it's not the solution to all issues with bpm handling.

Is it worth to clutter the preferences with this?

I wouldn't say so, no. I think it would be better not to implement it than to have to include a preference option.

@ronso0
Copy link
Member

ronso0 commented Jan 20, 2019

This might mask some cases where the analyzed BPM value isn't quite right, which could have some knock-on effects for bpm sync.

I think it would help to append special character(s) in this case to indicate there are decimals.
Maybe as simple as 124.., 124* or 124~. The decimals are visible in the decks so IMO this format could be the default, with a Preferences option to return to current display with all decimals visible.

@benis
Copy link
Contributor Author

benis commented Jan 20, 2019

This might mask some cases where the analyzed BPM value isn't quite right, which could have some knock-on effects for bpm sync.

I think it would help to append special character(s) in this case to indicate there are decimals.
Maybe as simple as 124.., 124* or 124~. The decimals are visible in the decks so IMO this format could be the default, with a Preferences option to return to current display with all decimals visible.

I thought about this too, but I felt that adding a non-numeric character would just clutter the field again. But if the idea is popular and the general consensus is that 124~ is better than 124.321 for example then I don't see why not. We could use ~124 and right-align that column, since the ~ character means 'approximately' in math terms anyway.

If there was going to be a Preferences option, I'd want to have the choice between:

  • Show all decimals
  • Show rounded numbers with indicator (eg 124~)
  • Show plain rounded numbers

@daschuer
Copy link
Member

For me this PR alone does not make any sense. The real issue is the BPM grouping.

@daschuer
Copy link
Member

Three decimal places are useless though. Maybe we can go for one or two.

@benis
Copy link
Contributor Author

benis commented Jan 20, 2019

I agree 2dp would be an improvement. If I made changes for that and pushed to this PR, would you still want to discuss further? If so we may as well get some further opinions now.

@daschuer
Copy link
Member

That will work for me without a preferences option.
We may also consider to add a .00 to the full BPM values to tidy up the column even more.

Consideration:
If we consider the normal rate slider range of 8% and a single precision rate slider and a average 120 BPM track, one slider tick is 0,08 BPM. This means the user can only control one decimal place in this case. A high precision slider can control 0,0006 though.

@ronso0 what do you think.

@benis
Copy link
Contributor Author

benis commented Jan 21, 2019

This is how forced 2 d.p. looks. Thoughts? It would be nice if the decimal points aligned, I don't know if Qt has that capability or not but I would guess it does.

screenshot 2019-01-21 at 19 48 43

@daschuer
Copy link
Member

I think a delegate can do this, like in #2002.
This will also allow to set the numbers of decimals by the columns width.

@benis
Copy link
Contributor Author

benis commented Jan 23, 2019

I think a delegate can do this, like in # 2002.
This will also allow to set the numbers of decimals by the columns width.

It will be slightly more tricky. There's already a bpm delegate that handles drawing the bpm lock etc, so I'll need to make it work around/with that. It would be good to get it working though, I'd be happy to use column width to set the decimal places.

@benis
Copy link
Contributor Author

benis commented Jan 26, 2019

I'll close this until I have something that needs review, so as not to add to the clutter of open PRs.

@benis benis closed this Jan 26, 2019
@ronso0
Copy link
Member

ronso0 commented Jan 26, 2019

I'll close this until I have something that needs review, so as not to add to the clutter of open PRs.

just leave it open as it's on a good way I think.
if the PR is not updated for a while even after a team member sends a reminder it'll get a stalled tag

@ronso0
Copy link
Member

ronso0 commented Jan 26, 2019

That will work for me without a preferences option.
We may also consider to add a .00 to the full BPM values to tidy up the column even more.

...and try to align the decimals point in all column fileds.

Consideration:
If we consider the normal rate slider range of 8% and a single precision rate slider and a average 120 BPM track, one slider tick is 0,08 BPM. This means the user can only control one decimal place in this case. A high precision slider can control 0,0006 though.

@ronso0 what do you think.

Users may have high-precision sliders but they usually don't have high-precision hands :)
And IF a DJ has high-precision ears to notice 0.01 BPM offsets they wouldn't rely on BPM display at all for beatmatching. That's just what I noticed when watching DJs experienced with vinyl when they play with digital gear.
having more than 3 decimals would be helpful -if at all- in the decks' rate section, but not in the library IMO.

@daschuer
Copy link
Member

So you vote is for one or two decimal places?

It is ok for me to merge this and do the decimal point allignment in a later PR.

@ronso0
Copy link
Member

ronso0 commented Jan 26, 2019

So you vote is for one or two decimal places?

IMO one decimal is enough for the library: we can see BPM is not an integer, the rest happpens in the decks.

@daschuer
Copy link
Member

@benis Ok, can you change it to one decimal place and the we are ready to merge. Thanks.

@benis benis reopened this Jan 27, 2019
@benis
Copy link
Contributor Author

benis commented Jan 27, 2019

Ok. For now do you care whether the number always has the decimal place or not? It's actually harder to display a number with 'up to' 1 d.p. than it is to just force 1 d.p.

You can use 'f' to set the number of decimal places or 'g' to set the number of significant figures, so using f would always give you xxx.y, but using g would give you (for example) xxx or xx.y depending on whether the bpm was >= 100 or < 100. So having the decimal place be optional is actually more work than just always displaying it.

@uklotzde
Copy link
Contributor

Personally I would prefer a format with a fixed number of decimal places. This is much easier to recognize. A single decimal place is a perfect compromise between readability and precision.

@benis
Copy link
Contributor Author

benis commented Jan 27, 2019

Sounds good to me. I think that's better in the case of numbers such as 121.04 anyway - without the decimal place this would just display as 121 which I think would be slightly misleading.

@ronso0
Copy link
Member

ronso0 commented Jan 27, 2019

I know this is out of scope for this PR but here's how I'd find the BPM column really useful, and it might be tricky if we use fonts with variable width (non-monospace fonts):

123
110.0
 97
 98.3
160
156

Slightly shrinking the column would result in

123
110.
 97
 98.
160
156

which allows to identify fast & slow tracks quickly, and also gives a hint when the BPM has decimals

@benis
Copy link
Contributor Author

benis commented Jan 27, 2019

Change to set bpm to 1 d.p. is done and pushed.

This is how it looks:
screenshot 2019-01-27 at 17 07 29

I'm tempted to modify this slightly in a future commit so that zero values are just left blank. Thoughts?

@benis
Copy link
Contributor Author

benis commented Jan 27, 2019

I know this is out of scope for this PR but here's how I'd find the BPM column really useful, and it might be tricky if we use fonts with variable width (non-monospace fonts):

123
110.0
 97
 98.3
160
156

Slightly shrinking the column would result in

123
110.
 97
 98.
160
156

which allows to identify fast & slow tracks quickly, and also gives a hint when the BPM has decimals

I'm actually planning to do something like this in the next PR. Currently the field elides by default, so when you shrink the column, unfortunately a value such as 123.0 immediately becomes 123…. With elide turned off and the decimal points aligned, you can use the column width in the way you've suggested. The tricky thing is that Qt currently manages the layout of the lock icon and bpm value automatically, and I think I'll have to do that manually in order to be able to do other stuff with the column. Should be doable though.

@uklotzde
Copy link
Contributor

uklotzde commented Jan 27, 2019

I didn't follow the discussions. But if we use a fixed number of decimal places then the column should be right-aligned as is common for displaying numeric values. The alignment helps when playing mixed music with a wide range of tempos between 50 and 190 bpm.

Usually I sort my genre/style/mood/phase crates by bpm, because this is the (technical) starting point for figuring out a next track that might fit. So for my personal workflow the number-style alignment is a nice-to-have feature.

@benis
Copy link
Contributor Author

benis commented Jan 27, 2019

Yeah, right-align would be a good choice to combine with this PR. I don't know if it can be done without having to rework the bpm delegate a fair bit, but I'll have a look.

@poelzi
Copy link
Contributor

poelzi commented Jan 28, 2019

For the music I play, the decimal point is a good indicator that the bpm detection failed. Nearly all songs with decimal points are wrongly detected tracks, It helps me not accidental play a track not matching. For me, 2 decimal points would nicely indicate that.

@benis
Copy link
Contributor Author

benis commented Jan 28, 2019

I'm wondering if it would be an idea to use an elide that's condition-based and only kicks in with narrower column widths. Currently, as soon as the elide kicks in you lose two characters, which is kind of a pain.

If it were possible to do a conditional elide, you could do something like this:

Value Wide display Medium display Narrow display Very narrow display
123 123.00 123 123 12…
123.04 123.04 123.0 123… 12…
123.004 123.00 123.0 123… 12…
122.96 122.96 123.0 122… 12…
119.96 119.96 120.0 119… 11…

Actually I'm not sure about the behaviour when it comes to rounding up. Maybe a simpler version would be better.

@benis
Copy link
Contributor Author

benis commented Feb 6, 2019

Replacing '0' values with a dash is a good look, imo:

screenshot 2019-02-06 at 22 13 06

@daschuer
Copy link
Member

daschuer commented Feb 7, 2019

I like the dash version.
I think this is already merge-able, right?
Should I merge this now or wait for the dash?

@benis
Copy link
Contributor Author

benis commented Feb 7, 2019

-edit- never mind, I will add the dash in shortly then this is good to merge.

@benis
Copy link
Contributor Author

benis commented Feb 7, 2019

Oh dear, fun with Git -_-

@benis benis reopened this Feb 7, 2019
@benis
Copy link
Contributor Author

benis commented Feb 7, 2019

Right then! Ready to merge :)

@daschuer
Copy link
Member

daschuer commented Feb 7, 2019

LGTM, Thank you.

@daschuer daschuer merged commit ae392c9 into mixxxdj:master Feb 7, 2019
@LindyBalboa
Copy link
Contributor

Hi, I just recompiled and I would like to say I'm not a fan of trailing zeros on integer values. I don't do any inter-track mixing so I don't need to be any more precise than integer BPMs. As it stands, this shows a useless xxx.0 for my whole library. I think I would be in favor of a config option to set the number of decimals displayed.

@ywwg
Copy link
Member

ywwg commented Feb 21, 2019

I have tracks that definitely need at least 2 decimal points of detail -- I assume this is just rounding the display? If I edit the bpm value will I see all the significant figures?

@daschuer
Copy link
Member

Correct.

@benis
Copy link
Contributor Author

benis commented Feb 21, 2019

I agree about the decimal places on integer values. The thing I'm working on currently would deal with that:

screenshot 2019-02-21 at 19 53 08

It combines a few changes currently, but if there's enough interest in it I could submit a PR that just addresses the trailing zeroes for integer values.

@LindyBalboa
Copy link
Contributor

It is not so urgent to me personally to have a fix in master. Your change was so atomic I can easily revert the commit. I'm just happy to hear that you are working further on it with this in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants