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

add WDoubleSpinbox: allow . and , in library BPM editor #13068

Draft
wants to merge 2 commits into
base: 2.5
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/widget/wcoverartmenu.cpp
src/widget/wcuemenupopup.cpp
src/widget/wdisplay.cpp
src/widget/wdoublespinbox.cpp
src/widget/weffectbuttonparametername.cpp
src/widget/weffectchain.cpp
src/widget/weffectchainpresetbutton.cpp
Expand Down
5 changes: 3 additions & 2 deletions src/library/bpmdelegate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <QTableView>

#include "moc_bpmdelegate.cpp"
#include "widget/wdoublespinbox.h"

// We override the typical QDoubleSpinBox editor by registering this class with
// a QItemEditorFactory for the BPMDelegate.
Expand All @@ -18,10 +19,10 @@ class BpmEditorCreator : public QItemEditorCreatorBase {
}

QWidget* createWidget(QWidget* parent) const override {
QDoubleSpinBox* pBpmSpinbox = new QDoubleSpinBox(parent);
WDoubleSpinBox* pBpmSpinbox = new WDoubleSpinBox(parent);
pBpmSpinbox->setFrame(false);
pBpmSpinbox->setMinimum(0);
pBpmSpinbox->setMaximum(1000);
pBpmSpinbox->setMaximum(99999);
Copy link
Member

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?

Copy link
Member Author

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);
Comment on lines 26 to 27
Copy link
Member

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?

Copy link
Member Author

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

pBpmSpinbox->setObjectName("LibraryBPMSpinBox");
Expand Down
25 changes: 25 additions & 0 deletions src/widget/wdoublespinbox.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#include "widget/wdoublespinbox.h"

#include <QLineEdit>
#include <QLocale>

#include "moc_wdoublespinbox.cpp"

WDoubleSpinBox::WDoubleSpinBox(QWidget* pParent)
: QDoubleSpinBox(pParent),
m_decSep(QLocale().decimalPoint()) {

Check failure on line 10 in src/widget/wdoublespinbox.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 22.04 (Qt 6.2, gcc)

no matching function for call to ‘QChar::QChar(QString)’
lineEdit()->setValidator(new QRegExpValidator(

Check failure on line 11 in src/widget/wdoublespinbox.cpp

View workflow job for this annotation

GitHub Actions / Ubuntu 22.04 (Qt 6.2, gcc)

expected type-specifier before ‘QRegExpValidator’
QRegExp("[0-9]{1,5}[." +
// add locale decimal point if it's not a dot
(m_decSep != '.' ? m_decSep : QChar()) +
Copy link
Member

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? 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"]?[0-9]{,8}"),
this));
}

// This gets the text from lineEdit()
double WDoubleSpinBox::valueFromText(const QString& text) const {
QString temp = text;
// replace , with dot before toDouble()
temp.replace(",", ".");
return temp.toDouble();
}
17 changes: 17 additions & 0 deletions src/widget/wdoublespinbox.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#pragma once

#include <QDoubleSpinBox>

// This spinbox accepts , and . as decimal separator which comes in handy when
// the used (typed) decimal separator doesn't match the locale's separator for
// some reason, e.g. when typing with numpads that have a fixed separator char.
class WDoubleSpinBox : public QDoubleSpinBox {
Q_OBJECT
public:
explicit WDoubleSpinBox(QWidget* pParent);

double valueFromText(const QString& text) const override;

private:
const QChar m_decSep;
};
Loading