-
-
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
add WDoubleSpinbox: allow . and , in library BPM editor #13068
base: 2.5
Are you sure you want to change the base?
Conversation
pBpmSpinbox->setFrame(false); | ||
pBpmSpinbox->setMinimum(0); | ||
pBpmSpinbox->setMaximum(1000); | ||
pBpmSpinbox->setMaximum(99999); |
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.
Any reason for allowing this much BPM?
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.
Obviously sky is the limit ; )
#12942 (comment)
pBpmSpinbox->setSingleStep(1e-3); | ||
pBpmSpinbox->setDecimals(8); |
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.
Not related to this PR but these settings feel overly precise, would you know if that something that was requested at some point or just arbitrary value that could be reviewed for a better UX?
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.
Yeah, 8 decimals look scary. I think there are two reason:
- for some tracks it seems it's not possible to match the BPM with
beats_adjust_slower/_faster
(kBpmAdjustStep
is 0.01), the spinbox allows finer adjusments - have a chance to discover that the BPM display is rounded when there are more than let's say 4 decimals
lineEdit()->setValidator(new QRegExpValidator( | ||
QRegExp("[0-9]{1,5}[." + | ||
// add locale decimal point if it's not a dot | ||
(m_decSep != '.' ? m_decSep : QChar()) + |
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.
Could it be worth to simply allow both ,
and .
? I know my current local would be .
but I would still use ,
as I grew up using ,
.
I guess the risk is that some users may type 1,000
for 1000
, but in the meantime, how often do you set the BPM value to > 999? 😅
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.
same as #13068 (comment)
Thanks for your review. |
This PR is marked as stale because it has been open 90 days with no activity. |
This PR is marked as stale because it has been open 90 days with no activity. |
currently only used in BPMDelegate.
Fixes #13051 for me.
I guess it's debatable whether we 'need' this, so this is a draft until we agreed on yes or no.
I'll probably look into making WBeatSpinBox equally permissive.