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

(fix) Library: BPM display uses decimal separator of selected locale #13067

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 7, 2024

fixes #13051

QString::number(double, format, precision) is locale-independent.

IMO this raises the question how we want to display BPM / beats in general:

  • Consistent according to selected locale?
    Need to fix WNumber, too.
    Maybe also dialogs like Track Properties ✔️ uses locale
    And WBeatSpinBox ✔️ uses locale
  • "International"? (decimal separator is . everywhere)
    Need to adjust library's BPM delegate (it's editor), since that currently correctly uses the selected locale (, or .)

The second commit fixes #13065

The max BPM limit may be moved to some prefs.h file and made available for BPMDelegate, WTrackProperties etc.

@daschuer
Copy link
Member

daschuer commented Apr 8, 2024

Since we translate Mixxx in various languages, also the decimal separator should be translated.

I can remember a discussion where the decimal comma has been considered as ugly in the bpm display in the skin.

@daschuer
Copy link
Member

daschuer commented Apr 8, 2024

I would vote for changing the library to localized decimal separator, and everywhere else. We have already the inconsistency between tooltips and grid element.

The number display in the skin could be still an issue, because the comma uses space below the bottom line of the numbers. For that reason I would vote keeping the decimal dot for all.

@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2024

I can remember a discussion where the decimal comma has been considered as ugly in the bpm display in the skin.

Yep. And I bet I was part of the comma group ; )

The widget commmit (WNumber etc.) was a test. I'm not sure, and I think I'll revert it for now (after others have taken a look).
But I think display and editor in the library it should be consistent. I can move the first commit (library) to a new PR or the second (widgets).

@daschuer
Copy link
Member

daschuer commented Apr 8, 2024

Yep. And I bet I was part of the comma group ; )

That would be definitely "correct". Do you have screenshots at hand, maybe the issue is no longer visible or we may default to native separator and allow overriding it by the skin.

@daschuer
Copy link
Member

daschuer commented Apr 8, 2024

The library commit can be merged as it is.

@ronso0 ronso0 force-pushed the bpm-display-locale branch from 241b35e to f7ed730 Compare April 8, 2024 12:54
@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2024

I removed f7ed730 and added a comment.
Ready to roll.

@ronso0
Copy link
Member Author

ronso0 commented Apr 8, 2024

I think #13068 is a good idea nonetheless to make the editor more permissive.

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, Thank you.

@daschuer daschuer merged commit 6dcc33c into mixxxdj:2.4 Apr 15, 2024
14 checks passed
@ronso0 ronso0 deleted the bpm-display-locale branch April 15, 2024 21:17
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.

2 participants