Implement switchable time format for decks#1652
Implement switchable time format for decks#1652poelzi wants to merge 5 commits intomixxxdj:masterfrom
Conversation
The display format on the decks can now be configured. Implement alternative display based on kiloseconds.
|
Thank you, nice Idea. Can you file a bug for this, than we can track this feature for a change log. |
|
tracking bug: https://bugs.launchpad.net/mixxx/+bug/1767179 |
| comboBoxTimeFormat->clear(); | ||
| comboBoxTimeFormat->addItem(tr("hh:mm:ss.zzz"), | ||
| static_cast<int>(TrackTime::DisplayFormat::DEFAULT)); | ||
| comboBoxTimeFormat->addItem(tr("k.sss:zz"), |
There was a problem hiding this comment.
I think we need here access QLocale::decimalPoint() and QLocale::groupSeparator().
I wonder if the colon matches a official time format for ks. Or should we use plain seconds instead like US k,sss.zz or German k.sss,zz?
There was a problem hiding this comment.
Or plain ks like use k.ssszz Or "k.sss zz" with space as group separator as defined in the SI system
https://en.wikipedia.org/wiki/Decimal_separator#Digit_grouping
There was a problem hiding this comment.
I like the decimal point as it makes longer tracks easier to read. "k.sss zz" sounds good for me, too.
Should the separator then be localized then ?
There was a problem hiding this comment.
Mmm I have just checked, currently I have the 00:00.00 format even in German. Now I can remember that we have discussed it before to use 00:00,00 for us, but this looks unusual, because all hardware devices have the dot, even though it would be correct.
When reading your PR I was stumbeling over "k.sss:zzz", just because is somehow ambiguous in this case.
7.332:84 Colon
7,332 84 Space
7,332 84 Punctuation Space
7,332 84 Thin Space
7,332 84 Hair Space
I think I prefer for now the localized thin Space version as recommended by SI-System.
|
|
||
| // Track Display model | ||
| comboBoxTimeFormat->clear(); | ||
| comboBoxTimeFormat->addItem(tr("hh:mm:ss.zzz"), |
There was a problem hiding this comment.
here we also need to insert the QLocale::decimalPoint()
| static_cast<int>(TrackTime::DisplayFormat::KILO_SECOND)); | ||
| connect(comboBoxTimeFormat, SIGNAL(activated(int)), | ||
| this, SLOT(slotTimeFormatChanged(int))); | ||
| slotTimeFormatChanged((double)m_pConfig->getValue(ConfigKey("[Controls]", "TimeFormat"), |
There was a problem hiding this comment.
use static_cast, but I think you can omit it in this case
| m_pConfig->set(ConfigKey("[Controls]","TimeFormat"), ConfigValue(v)); | ||
| comboBoxTimeFormat->setCurrentIndex( | ||
| comboBoxTimeFormat->findData(i)); | ||
|
|
|
|
||
| void DlgPrefDeck::slotTimeFormatChanged(int index) { | ||
| int mode = comboBoxTimeFormat->itemData(index).toInt(); | ||
| m_timeDisplayFormat = static_cast<TrackTime::DisplayFormat>(mode); |
There was a problem hiding this comment.
I think you can omit the m_timeDisplayFormat here and read the comboBoxTimeFormat directly
| } | ||
|
|
||
| void DlgPrefDeck::slotApply() { | ||
| double timeDisplay = static_cast<double>(m_timeDisplayMode); |
There was a problem hiding this comment.
I think we can get also rid of m_timeDisplayMode
|
|
||
| } | ||
|
|
||
| void DlgPrefDeck::slotTimeFormatChanged(int index) { |
There was a problem hiding this comment.
I think we should get rid of this. It is anyway confusing to have this function twice as int and double overload.
| ElapsedAndRemaining, | ||
| }; | ||
| enum class DisplayFormat { | ||
| DEFAULT, |
There was a problem hiding this comment.
DEFAULT is meaningless here. How about WALL_CLOCK or HOUR_MINUTES_SECONDS or TRADITIONAL
|
|
||
| QString durationString = | ||
| (days > 0 ? (QString::number(days) % | ||
| QLatin1String("d, ")) : QString()) % |
There was a problem hiding this comment.
Why do we need to deal with days? Can't we just count seconds?
There was a problem hiding this comment.
I just took the logic of the normal formatting here. I don't think tracks over a 1 day are so common ;)
There was a problem hiding this comment.
I think we schould remove the day part entirely. No track will have this length, and if there is what we have unlikely the screen space for this.
I personally would prefer to see
30:10:01,86 instead of 1d,6:10:01,86
| QString durationString = | ||
| (days > 0 ? (QString::number(days) % | ||
| QLatin1String("d, ")) : QString()) % | ||
| QString("%1.%2").arg(kilos, 0, 10).arg(seconds, 3, 'f', 0, QLatin1Char('0')); |
There was a problem hiding this comment.
QLocale::toString() can already do this in the correct format.
Fix comments regarding style. Also add hectosecond formatting. For mixing, 100 seconds is actually a quite useful seperator.
|
I tried localizing the time format and the results where terrible. In fact, we don't localize the time at all nor do I think we should. Nobody complained about the old format being not localized and I think it should be presented in the best readable way. Which it is for normal time format. I tried different options for the kilo second and new hecto second displays. The period combined with the small space look the best. The german "," looks super strange. In fact, I found the hecto second display quite appealing. Most tracks are in the 5-10 minute range. 100 seconds are a good marker for the track end, so the first diget after the separator is the important one. |
| } // namespace | ||
|
|
||
| // static | ||
| QString DurationBase::kCentisecondSeperator = QString::fromUtf8("\u2009"); |
There was a problem hiding this comment.
This requires a comment.
// Unicode for THIN SPACE
I am also a bit confused why it works. The UTF8 character is 0xe28089
"\u2009" should be already UTF-16, the internal codec of QString. Does the compiler turns it into utf8 and than fromUtf8 back at rune time?
Did you try QChar(0x2009); ?
|
It works nice. Should the library column also follow the settings? |
|
A Beats time format or better bars, if we have it one day would also be a nice benefit. |
rename formatSeconds into formatTime to reflect function better.
|
Ready for review. Beats counter sounds interesting. |
| } | ||
|
|
||
| QString DurationBase::formatSeconds(double dSeconds, Precision precision) { | ||
| if (dSeconds < 0.0) { |
There was a problem hiding this comment.
Use VERIFY_OR_DEBUG_ASSERT macro
|
|
||
| // static | ||
| QString DurationBase::formatKiloSeconds(double dSeconds, Precision precision) { | ||
| if (dSeconds < 0.0) { |
There was a problem hiding this comment.
use VERIFY_OR_DEBUG_ASSERT macro
|
|
||
| // static | ||
| QString DurationBase::formatHectoSeconds(double dSeconds, Precision precision) { | ||
| if (dSeconds < 0.0) { |
There was a problem hiding this comment.
use VERIFY_OR_DEBUG_ASSERT macro
|
|
||
| // static | ||
| QString DurationBase::formatTime(double dSeconds, Precision precision) { | ||
| if (dSeconds < 0.0) { |
There was a problem hiding this comment.
use VERIFY_OR_DEBUG_ASSERT macro
There was a problem hiding this comment.
@Be-ing when I replace these I get Critical [Main]: DEBUG ASSERT: "dSeconds < 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:26 when I attempt to run. Any ideas?
|
Ping @poelzi. Could you make the small edits mentioned above? I'd like to merge this by Saturday for 2.2. |
|
@poelzi would you be happy for somebody else to pick this up and finish it off? |
|
@Be-ing I was going to pick this up and submit a new PR. The VERIFY_OR_DEBUG_ASSERT macro, is it just a case of replacing the instances of 'if' with that? Or do I need to add in a qDebug statement somewhere. |
|
Printing with |
|
I can't get this to build. I get a dozen or so errors, none of which seem related (at least not directly) to the files that were changed: Any idea why that would happen? I completely cleaned beforehand including running |
|
@beenisss can you post the full compiler command and output for e.g. analysisfeature.o ? |
|
Here you go, I think: |
| #include <QStringBuilder> | ||
| #include <QTime> | ||
|
|
||
| #include <math.h> |
There was a problem hiding this comment.
I'm a bit confused by this addition. It has no path and the syntax doesn't match the other entries. Besides which, two lines below is the statement #include "util/math.h" - is that a different file?
There was a problem hiding this comment.
"util/math.h" (in mixxx's codebase) is preferable to <math.h> (the C standard library header), since it includes Mixxx-specific helpers / shims.
Those aren't the same errors you were seeing before? |
|
I'm not sure what you mean, I think I've probably misunderstood what you asked for or have misled you with what I originally posted. I'm currently working on reimplimenting this step by step and building as I go. I've noticed a couple of weird things in the updated code in the process of doing so, maybe once those are ironed out it'll work. I'm happy to post up anything that's useful for troubleshooting in the meantime though. |
…/update duration.cpp
…pp, if > VERIFY_OR_DEBUG_ASSERT for duration.cpp
|
Well, I've applied the updates from this PR to a new branch manually with some changes that were minor and almost entirely cosmetic, and somehow it builds without issue. ¯\_(ツ)_/¯ |
|
In my personal branch I've changed Can we add another format |
|
In light of the above I think this can be closed. |
|
When I developed this, I tried different characters. I think I spent an hour trying different combinations for this not really usually formats. I chose those because after giving each version a try of 10 minutes or so, I found those ok. I'm quite open to some better ideas :) |
|
I can build this branch merged to master. @benis What is your issue? Appveyor is also able to build this. However some tests fails: Travis has timed out. |
…/update duration.cpp
…/update duration.cpp
|
closed in favor of #1985 |
Reimplement #1652 - switchable time formats
7 participants
The display format on the decks can now be configured.
Implement first alternative display based on kiloseconds.
If you mixx tracks based on first and last downbeat, it is unnecessary complicated to use minutes. The first alternative display shows "k.sss:zzz" following roughly the idea of the kilosecond so it can be much easier calculated when the next track should be started.