Reimplement #1652 - switchable time formats by daschuer · Pull Request #1985 · mixxxdj/mixxx

daschuer · 2019-01-06T21:28:48Z

This is a rebased Version of #1915 onto the original #1652
to keep the original commit messages.

The display format on the decks can now be configured. Implement alternative display based on kiloseconds.

Fix comments regarding style. Also add hectosecond formatting. For mixing, 100 seconds is actually a quite useful seperator.

rename formatSeconds into formatTime to reflect function better.

daschuer · 2019-01-06T22:15:31Z

LGTM, waiting for CI

benis · 2019-01-07T07:00:49Z

There is still some work to do on this. Is there a way for me to push to this PR?

Also, the tests you said were failing - how do you run those?

daschuer · 2019-01-07T08:26:52Z

You can issue a new pull request against this branch in my daschuer/mixxx repro.
Or you issue your own pr and we close this.

You can run the test on Linux by starting
./mixxx-test

The verify debug assert was wrong because we use it only for conditions that never happens. But it happens at least during the test. I am not sure if it happens during Mixxx with faulty tracks for example.

daschuer · 2019-01-07T08:29:15Z

You can get this by:
git checkout -b hectoseconds2 upstream/master
git pull https://github.com/daschuer/mixxx.git hectoseconds

daschuer · 2019-01-10T07:26:26Z

@benis: Is it possible to finish this by the end of January?
I like to see this in Mixxx 2.3. Or can we merge this step as it is?
Which changes do you have planned?

benis · 2019-01-10T21:16:21Z

I would hope to have this done by the end of January for sure.

I've just created a PR to your hectoseconds branch with the change for Traditional (Coarse) time format.

benis · 2019-01-10T21:38:17Z

I still need to check if the tests are working and figure out why not if not. If you can merge the PR I made to your hectoseconds branch, the tests are the only critical thing before this could be merged.

Here are some other things I would like to look at:

Can we find a more conventional choice of separator character?

Also, the options displayed in the Deck Preferences are a bit inconsistent:

is there a way to indicate that the hh is optional, or could we just remove it? Not many tracks over an hour long.
's zz' uses the defined centisecond separator character (thin space) - should it?
'k.sss zz' uses a static period, even though the actually-displayed character might be different. Then the defined centisecond separator character is used between sss and zz
'hs.ss zz' also has a static period but uses the hectosecond separator character between ss and zz
(See dlgprefdeck.cpp:122 onwards for relevant code.)

I also noticed that if you choose a different time format in Preferences but then hit Cancel, when you next load Preferences the drop-down has retained your value, so if you go back in to change something else, the deck preferences then get applied too.

benis · 2019-01-10T21:51:29Z

Just ran the tests, if I've understood the output correctly they now pass.

daschuer · 2019-01-11T14:53:25Z

Formating:
Let's follow strictly the SI recommendation in en-US format.
Decimal separator .
Thouthands colon ,
Decimal places seperator thin space.

Seconds ss,sss.ss
Kiloseconds ss.sss ss
Hectoseconds sss.ss ss

For the hh: I think a minute only format is also common. A movie runtime is given as 98 min for example. So we can pick IMHO that and drop the hh: entirely.

…ences

benis · 2019-01-11T19:48:20Z

I don't think your examples match up with SI. The SI standard says that decimal places can be separated by a point or a comma (the former in US), and that the thousand separator should be a space. That makes sense to me, could we also use it as the hundred separator in the hectoseconds format?

The choice of variable names for separator characters (both 'group' separator characters and decimal separator), as well as the actual application of formatting, was inconsistent and confusing. Now it's better, hopefully.

benis · 2019-01-13T09:45:40Z

Traditional OK

Traditional (Coarse) OK

Seconds OK

Kiloseconds NOK
* This is part of a kiloseconds movement https://kilosecond.info/ the

* I think it should be 0.000 62  -1.000 95
Hectoseconds I don't care much
* I think it should be 0.00 62  -10.00 95 according to SI.

I still don't think that's consistent with SI, but feel free to change it.

Additional minor fixes

daschuer · 2019-01-14T09:24:57Z

I still don't think that's consistent with SI, but feel free to change it

I can do this. Why do you think my proposal is not SI?
IMHO the decimal point needs to shifted with kilo and hecto.

benis · 2019-01-14T19:47:01Z

https://en.wikipedia.org/wiki/International_System_of_Units#General_rules

The symbol for the decimal marker is either a point or comma on the line
...
Spaces should be used as a thousands separator (1 000 000) in contrast to commas or periods (1,000,000 or 1.000.000) to reduce confusion resulting from the variation between these forms in different countries.

So spaces for thousands, point or comma for decimal marker. You're proposing to do the opposite.

benis · 2019-01-14T19:48:14Z

 class DurationBase {

  public:
+


I seem to have introduced this empty line in error.

daschuer · 2019-01-19T16:33:49Z

So spaces for thousands, point or comma for decimal marker. You're proposing to do the opposite.

I read this like this:
1 ks = 16 min + 40 s
1 000 ks = 11 d + 13 h + 64 min + 40 s
0.9 ks = 15 min
0.001 ks = 1 s
0.000 001 hs = 1 ms

1 hs = 1 min + 40 s
1 000 hs = 27 h + 46 min + 40 s
0.9 hs = 1.5 min
0.001 hs = 100 ms
0.000 01 hs = 1 ms

benis · 2019-01-19T17:14:52Z

Ah, I see where you're coming from now, sorry for the confusion. Are you happy to change it? It will be less faff than me submitting another PR to you.

daschuer · 2019-01-19T17:31:09Z

OK I will do the change, I am only unsure about hecto seconds.
should we go with the SI correct:
0.000 1 hs = 10 ms
with the nicer looking:
0.00 01 hs = 10 ms

benis · 2019-01-19T17:49:31Z

I vote for the second one. It's kind of an unconventional format so why not.

daschuer · 2019-01-19T20:01:38Z

OK, done.

However, I have just played with it and it feels odd.
I am considering to replace it with a plain seconds format with three leading zeros.
000.00

Or should we reintroduce your original hectoseconds format?
What do you think?

benis · 2019-01-19T21:38:52Z

I'm not attached to the hectoseconds format, so I wouldn't mind if we replaced it. I think your suggestion is a better choice and more likely to get used.

daschuer · 2019-01-19T21:41:24Z

OK I will change it.

…o hard to read and the new long format is basically the same but with other separators

daschuer · 2019-01-20T00:04:10Z

Done. What else is missing before merge?

benis · 2019-01-20T11:47:09Z

I think that's everything. I inadvertently introduced a blank line in src/util/duration.h (:17) so I guess this would be a good time to fix that, but otherwise this is good to go.

daschuer · 2019-01-20T17:58:55Z

OK, I am fine with this now.
We may consider to alter also the library column accordingly, but that is a separate PR.

@uklotzde Is anything left to do before merge?

daschuer · 2019-01-27T20:51:47Z

OK time to merge this.

daschuer · 2019-01-27T20:52:58Z

Thank you @benis and @poelzi for working on this.

uklotzde · 2019-05-25T15:27:55Z

New bug: https://bugs.launchpad.net/mixxx/+bug/1830488

poelzi and others added 10 commits April 26, 2018 19:40

Implement switchable duration format

9c658a0

The display format on the decks can now be configured. Implement alternative display based on kiloseconds.

Cleanup and Hectoseconds

66fc9d1

Fix comments regarding style. Also add hectosecond formatting. For mixing, 100 seconds is actually a quite useful seperator.

add simple second display.

f987e3d

rename formatSeconds into formatTime to reflect function better.

fixup tests. Try some unrelated character as seperator in hectoseconds.

f6f2a78

Merge remote-tracking branch 'origin/master' into feature/seconds

b8b0076

Merge commit 'f325270ee31bcdad96e1f206637f059ad72e6b01' into seconds

c77bd6a

Implement switchable time format for decks by @poelzi - mixxxdj#1652

22a2b4c

Comment to identify separator characters

a87dc95

Fix issues identified by CodeFactor

5f1196c

Replace if with VERIFY_OR_DEBUG_ASSERT in src/util/duration.cpp

5dc62e6

daschuer mentioned this pull request Jan 6, 2019

[WIP] Reimplement #1652 - switchable time formats #1915

Closed

reintroduce the lost "?" for negative durations

7685e0a

daschuer force-pushed the hectoseconds branch from 1d7567e to 7685e0a Compare January 6, 2019 22:13

daschuer mentioned this pull request Jan 6, 2019

Implement switchable time format for decks #1652

Closed

benis reviewed Jan 7, 2019

View reviewed changes

Comment thread src/util/duration.cpp

benis added 2 commits January 10, 2019 21:01

Add 'Traditional (Coarse)' time format option

3efd74d

Minor formatting corrections to duration.cpp

c0d3cae

Re-format dlgprefdeck.cpp:120 onwards to make more readable

5fe1f0f

Remove 'hh:' from descrptions in time format drop-down in Deck Prefer…

341242a

…ences

Clean up deck time formatting logic

f27e92d

The choice of variable names for separator characters (both 'group' separator characters and decimal separator), as well as the actual application of formatting, was inconsistent and confusing. Now it's better, hopefully.

Merge remote-tracking branch 'upstream/master' into hectoseconds

407164a

benis added 2 commits January 13, 2019 10:05

Refactor DisplayMode enum for consistency

9977df3

Return const QString for invalid duration value

5ba36de

benis mentioned this pull request Jan 13, 2019

Additional minor fixes daschuer/mixxx#38

Merged

Merge pull request #38 from benis/hectoseconds2

19cf23d

Additional minor fixes

benis reviewed Jan 14, 2019

View reviewed changes

Comment thread src/util/duration.h

class DurationBase {

public:

Copy link
Copy Markdown

Contributor

benis Jan 14, 2019

Choose a reason for hiding this comment

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

I seem to have introduced this empty line in error.

make time formats more like SI

5d357b2

Replace hectoseconds format by seconds long. Hectosends fromat was to…

43aeb9e

…o hard to read and the new long format is basically the same but with other separators

daschuer merged commit 43dc870 into mixxxdj:master Jan 27, 2019

ronso0 referenced this pull request Jan 30, 2019

fix minimal required width for tracktime

bd14722

nopeppermint mentioned this pull request Jan 30, 2019

adapt visible Skin Settings in small Deck in Latenight #2011

Closed

daschuer deleted the hectoseconds branch September 7, 2021 21:09

This was referenced Aug 23, 2022

Tracking: switchable time formats for decks #9268

Closed

Time format changes unintentionally #9665

Closed

Uh oh!

Conversation

daschuer commented Jan 6, 2019

Uh oh!

daschuer commented Jan 6, 2019

Uh oh!

Uh oh!

benis commented Jan 7, 2019

Uh oh!

daschuer commented Jan 7, 2019

Uh oh!

daschuer commented Jan 7, 2019

Uh oh!

daschuer commented Jan 10, 2019

Uh oh!

benis commented Jan 10, 2019

Uh oh!

benis commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benis commented Jan 10, 2019

Uh oh!

daschuer commented Jan 11, 2019

Uh oh!

benis commented Jan 11, 2019

Uh oh!

benis commented Jan 13, 2019

Uh oh!

daschuer commented Jan 14, 2019

Uh oh!

benis commented Jan 14, 2019

Uh oh!

benis Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

daschuer commented Jan 19, 2019

Uh oh!

benis commented Jan 19, 2019

Uh oh!

daschuer commented Jan 19, 2019

Uh oh!

benis commented Jan 19, 2019

Uh oh!

daschuer commented Jan 19, 2019

Uh oh!

benis commented Jan 19, 2019

Uh oh!

daschuer commented Jan 19, 2019

Uh oh!

daschuer commented Jan 20, 2019

Uh oh!

benis commented Jan 20, 2019

Uh oh!

daschuer commented Jan 20, 2019

Uh oh!

daschuer commented Jan 27, 2019

Uh oh!

daschuer commented Jan 27, 2019

Uh oh!

uklotzde commented May 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benis commented Jan 10, 2019 •

edited

Loading