MBS-5765: Correctly render a duration in seconds as hours, minutes, seconds #15

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

ocharles commented Jan 22, 2013

Time::Duration->normalize normalizes a large duration into years, months, and
days - rather than just the needed hours, minutes, seconds. Rather than manually
re-fudging the calculation to account for that overflow, I've just implemented
partioning of seconds in hours, minutes and seconds manually.

@ocharles ocharles MBS-5765: Correctly render a duration in seconds as hours, minutes, s…
…econds

Time::Duration->normalize normalizes a large duration into years, months, and
days - rather than just the needed hours, minutes, seconds. Rather than manually
re-fudging the calculation to account for that overflow, I've just implemented
partioning of seconds in hours, minutes and seconds manually.
63289a1

@ianmcorvidae ianmcorvidae commented on the diff Jan 22, 2013

lib/MusicBrainz/Server/Track.pm
@@ -23,12 +23,18 @@ sub FormatTrackLength
return $ms unless looks_like_number($ms);
return "$ms ms" if $ms < 1000;
- my $seconds = $ms / 1000.0 + 0.5;
@ianmcorvidae

ianmcorvidae Jan 22, 2013

Contributor

If you're getting rid of this you need to add 500 to the $ms number to keep this consistent.

@ocharles

ocharles Jan 25, 2013

Contributor

Ok, it was unclear why that was even there - 7fcda88 isn't much help on why it's there either.

@ianmcorvidae

ianmcorvidae Jan 25, 2013

Contributor

Ah, okay. The reason is that using floor() (as we do, here and in the JS version of this function both) means that something like 3700ms would get rounded to 3 seconds, when it's actually closer to 4 seconds. So, by adding 500ms (or 0.5s) to the number, floor() ends up working like you'd expect with millisecond values between 500 and 999 above a given thousand.

@ocharles

ocharles Jan 25, 2013

Contributor

Oh, so maybe it would be better to use round, which seems to be what we want?

@warpr

warpr Jan 26, 2013

Owner

It does seem that round is what we want :)
Though if you change that here I would prefer you also change the implementation of the javascript equivalent (in common/MB/utility.js).

Contributor

ianmcorvidae commented Jan 29, 2013

Looks good to me, so long as tests still pass. 🚢 if so!

ocharles closed this Feb 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment