-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
option to show both elapsed and remaining time simultaneously #1216
Conversation
@@ -482,6 +482,16 @@ | |||
</attribute> | |||
</widget> | |||
</item> | |||
<item row="9" column="3"> |
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 introduction of a 3rd column throws off the positioning/aligning of the existing options. Instead, like with the tooltips options, use a horizontal layout for the time display options.
src/widget/wnumberpos.cpp
Outdated
if (dPosSecondsElapsed >= 0.0) { | ||
setText(mixxx::Duration::formatSeconds( | ||
dPosSecondsElapsed, mixxx::Duration::Precision::CENTISECONDS) | ||
% QLatin1String(" / -") % |
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.
To me the /
separator is unneeded.
Depending on the font, i found it harder to read with it, we already have the -
in front of the remaining time display as separator, so 3 spaces and -
should do it.
Thanks for adding a long standing request. Edit: |
The minus sign ("-") before the remaining time is enough.
LGTM |
@@ -70,9 +70,14 @@ DlgPrefControls::DlgPrefControls(QWidget * parent, MixxxMainWindow * mixxx, | |||
if (!m_pConfig->exists(ConfigKey("[Controls]","PositionDisplay"))) | |||
m_pConfig->set(ConfigKey("[Controls]","PositionDisplay"),ConfigValue(0)); | |||
|
|||
if (m_pConfig->getValueString(ConfigKey("[Controls]", "PositionDisplay")).toInt() == 1) { | |||
int positionDisplayType = m_pConfig->getValueString( |
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.
You can combine this line and the line above by
int positionDisplayType = m_pConfig->getValue(ConfigKey("[Controls]", "PositionDisplay"), 0);
if (m_pConfig->getValueString(ConfigKey("[Controls]", "PositionDisplay")).toInt() == 1) { | ||
int positionDisplayType = m_pConfig->getValueString( | ||
ConfigKey("[Controls]", "PositionDisplay")).toInt(); | ||
if (positionDisplayType == 1) { | ||
radioButtonRemaining->setChecked(true); | ||
m_pControlTrackTimeDisplay->set(1.0); |
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.
Please use constexpr for these magic numbers.
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 don't understand how a constexpr would help here. Do you mean an enum class?
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.
You cannot convert a enum class to a double without a cast.
The idea was to define
constexpr double k... = 1.0
to get around this.
Everything that replaces the 1.0 by a readable text works for me here, I do not care much about type safety here.
You already use a private enum class inside the widget with the same values. Maybe you find common solution for both.
Tested on Shade, Deere and LateNight, LGTM |
Clang build fails:
|
build was failing for Mac OS X
Build fixed on all platforms. |
Ready for merge? |
@@ -70,12 +70,23 @@ DlgPrefControls::DlgPrefControls(QWidget * parent, MixxxMainWindow * mixxx, | |||
if (!m_pConfig->exists(ConfigKey("[Controls]","PositionDisplay"))) | |||
m_pConfig->set(ConfigKey("[Controls]","PositionDisplay"),ConfigValue(0)); |
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.
Here we should also use the TrackTime::DisplayMode class.
This value does not match the value set in slotResetToDefaults()
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 is a common issue whenever someone works on a preferences dialog. Perhaps as part of @Pegasus-RPG's refactoring of the preferences we can come up with a better solution.
Thank you for working on that (and all the other stuff) |
…apsed and remaining time simultaneously mixxxdj/mixxx#1216
Add a third option for WNumberPos to show both the elapsed and remaining time on a deck. When this is chosen, the format is "elapsed time / remaining time".