[WIP] Reimplement #1652 - switchable time formats

[benis]

I've reapplied the changes from #1652 as I could not get that PR to build. I suspect it was changes to other bits of code in the changed files that created an issue, as this PR builds for me.

Things still outstanding:

• The review for the previous PR requested several if statements in src/util/duration.cpp be replaced with VERIFY_OR_DEBUG_ASSERT. When I attempted to apply these changes I got Critical [Main]: DEBUG ASSERT: "dSeconds < 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:xx errors at runtime. I don't know what to do about this.
• Choice of separator characters in src/util/duration.cpp. For centiseconds this is currently set to a thin space, for hectoseconds this is currently set to ⌞ (the unicode character for bottom left corner.) These aren't my choices and I wonder if we can't find some more logical ones.
• This request from ronso0:

In my personal branch I've changed mixxx::Duration::Precision::CENTISECONDS to ...::SECONDS in /src/widget/wnumberpos.cpp to get rid of the rather distracting centiseconds.

Can we add another format hh:mm:ss and maybe call it "Traditional, coarse"?

[daschuer]

@benis Thank you for jumping in here. Please rebase your work on top of the original commits of @poelzi #1652

It would be also nice if you can sqash your commits and add commit messages describing your work.

A rebased version with resolved conflicts is here:
https://github.com/daschuer/mixxx/tree/hectoseconds
You may use
git rebase -i
to improve the commit messages by
git commit --ammend

[daschuer]

Thanks

[benis]

I'm a bit confused.

1. What does rebasing achieve?
2. You asked in the other PR what my issue with building that PR was. That's covered by this comment and the ones below it, I think. But if it builds for you maybe that's not a problem.
3. In any case I assume only one of the PRs need to be open. I made this one as poelzi didn't respond for a while on the other one and there were some minor things that needed correcting. Shall we pick one of them so the discussion is all in one place?
4. I think we still need a solution for this issue, unless that's another weird local issue on my machine and isn't affecting you:

• The review for the previous PR requested several if statements in src/util/duration.cpp be replaced with VERIFY_OR_DEBUG_ASSERT. When I attempted to apply these changes I got Critical [Main]: DEBUG ASSERT: "dSeconds < 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:xx errors at runtime. I don't know what to do about this.

[daschuer]

Oh, I am sorry I did not explain the whole issue.

What does rebasing achieve?

We rely on the Git history to track the copyright owner of the source. This is relevant if we ever want to reuse some potions of code with a non GPL 2.0 compatible license. In this case we need to ask the copyright owner for permission. That's why I like to stick with the original commits and commit your changes on top of that.

You asked in the other PR what my issue with building that PR was. That's covered by this comment and the ones below it, I think. But if it builds for you maybe that's not a problem.

OK, so it looks like you had unrelated issue like scons cache issues or such? Do you have a clue what actually fixes the issue for you?

In any case I assume only one of the PRs need to be open. I made this one as poelzi didn't respond for a while on the other one and there were some minor things that needed correcting. Shall we pick one of them so the discussion is all in one place?

It is OK to continue here. I don't care if the original one is still open or not. Since this is almost ready, we can merge it soon anyway and close both PRs in turn.

I think we still need a solution for this issue, unless that's another weird local issue on my machine and isn't affecting you.

ASSERTS are for the impossible conditions, just to check if this assumption is true and stays true during the lifetime of the source. In you case the assumption is wrong. So it is either a wrong assumption or we have found a bug. Do you have a clue what is the case in the failing code?

[benis]

Ok I've had a look at the build issues. The immediate reason #1652 won't build for me is for some reason related to changes that were made to the includes in a completely different PR, #1683.

All the build errors I've been seeing relate to files that were changed in that PR. If I apply the changes from that PR on top of the changes in 1652 it builds fine.

Obviously when I manually applied the changes to create this PR, those changes were already merged, which explains the discrepancy, but why would I have a problem building this but not you? To checkout the PR I am using git fetch upstream pull/1652/head:1652 to set it up in its own branch on my local machine.

[benis]

Ping @daschuer. I think I have identified what is causing the test fails and the errors triggered by VERIFY_OR_DEBUG_ASSERT, so I am working on those.

Edit: not sure the below actually matters. If you can build this branch then I think we can ignore it.

The immediate reason #1652 won't build for me is for some reason related to changes that were made to the includes in a completely different PR, #1683.

All the build errors I've been seeing relate to files that were changed in that PR. If I apply the changes from that PR on top of the changes in 1652 it builds fine.

Obviously when I manually applied the changes to create this PR, those changes were already merged, which explains the discrepancy, but why would I have a problem building 1652 but not you? To checkout the PR I am using git fetch upstream pull/1652/head:1652 to set it up in its own branch on my local machine.

[benis]

Regarding commit 0e0a798, the if statement was checking for the failure condition: if (dSeconds < 0.0) { and if I've understood correctly, VERIFY_OR_DEBUG_ASSERT should check for the pass condition. Therefore I've updated those lines from if (dSeconds < 0.0) { to VERIFY_OR_DEBUG_ASSERT (dSeconds >= 0.0) { and now it works.

[daschuer]

I have just issued #1985 as a rebased version of this.

unfortunately the test is still failing:

[ RUN      ] DurationUtilTest.FormatSecondsNegative
DEBUG ASSERT: "dSeconds >= 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:28
src/test/durationutiltest.cpp:98: Failure
Value of: mixxx::Duration::formatTime(-1, mixxx::Duration::Precision::SECONDS)
  Actual: { 2-byte object <32-00>, 2-byte object <33-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00> }
Expected: "?"
Which is: 0xe6df51
DEBUG ASSERT: "dSeconds >= 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:28
src/test/durationutiltest.cpp:99: Failure
Value of: mixxx::Duration::formatTime(-1, mixxx::Duration::Precision::CENTISECONDS)
  Actual: { 2-byte object <32-00>, 2-byte object <33-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00>, 2-byte object <2E-00>, 2-byte object <30-00>, 2-byte object <30-00> }
Expected: "?"
Which is: 0xe6df51
DEBUG ASSERT: "dSeconds >= 0.0" in function static QString mixxx::DurationBase::formatTime(double, mixxx::DurationBase::Precision) at src/util/duration.cpp:28
src/test/durationutiltest.cpp:100: Failure
Value of: mixxx::Duration::formatTime(-1, mixxx::Duration::Precision::MILLISECONDS)
  Actual: { 2-byte object <32-00>, 2-byte object <33-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00>, 2-byte object <3A-00>, 2-byte object <35-00>, 2-byte object <39-00>, 2-byte object <2E-00>, 2-byte object <30-00>, 2-byte object <30-00>, 2-byte object <30-00> }
Expected: "?"
Which is: 0xe6df51
[  FAILED  ] DurationUtilTest.FormatSecondsNegative (0 ms)

[daschuer]

I assume we use -1 duration for unknown so returning "?" might be reasonable.
I am not sure though.

[daschuer]

Fixed here: #1985

[daschuer]

I will close this now in favor of #1985