Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 2 commits into from

3 participants

@ocharles
Owner

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
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 Owner

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

@ocharles Owner

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

@ianmcorvidae Owner

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 Owner

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

@warpr Owner
warpr added a note

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).

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

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

@ocharles ocharles closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 22, 2013
  1. @ocharles

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

    ocharles authored
    …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.
Commits on Jan 29, 2013
  1. @ocharles
This page is out of date. Refresh to see the latest.
View
18 lib/MusicBrainz/Server/Track.pm
@@ -1,7 +1,7 @@
package MusicBrainz::Server::Track;
use strict;
use Carp 'confess';
-use aliased 'DateTime::Format::Duration';
+use POSIX qw( floor );
use Scalar::Util qw( looks_like_number );
use Sub::Exporter -setup => {
@@ -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 Owner

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

@ocharles Owner

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

@ianmcorvidae Owner

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 Owner

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

@warpr Owner
warpr added a note

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ my $one_second = 1000.0;
+ my $one_minute = $one_second * 60;
+ my $one_hour = $one_minute * 60;
- my %data = Duration->new->normalise (seconds => $seconds);
- return $data{hours} > 0 ?
- sprintf ("%d:%02d:%02d", $data{hours}, $data{minutes}, $data{seconds}) :
- sprintf ("%d:%02d", $data{minutes}, $data{seconds});
+ my ($hours, $minutes, $seconds);
+ ($hours, $ms) = (floor($ms / $one_hour), $ms % $one_hour);
+ ($minutes, $ms) = (floor($ms / $one_minute), $ms % $one_minute);
+ $seconds = round($ms / $one_second);
+
+ return $hours > 0 ?
+ sprintf ("%d:%02d:%02d", $hours, $minutes, $seconds) :
+ sprintf ("%d:%02d", $minutes, $seconds);
}
sub FormatXSDTrackLength
View
34 root/static/scripts/common/MB/utility.js
@@ -197,29 +197,29 @@ MB.utility.formatTrackLength = function (duration)
return duration + ' ms';
}
- var seconds = 1000;
- var minutes = 60 * seconds;
- var hours = 60 * minutes;
+ var one_second = 1000.0;
+ var one_minute = 60 * one_second;
+ var one_hour = 60 * one_minute;
- var hours_str = '';
- duration = duration + 500;
+ var hours = Math.floor(duration / one_hour);
+ duration = duration % one_hour;
- if (duration > 1 * hours)
- {
- hours_str = Math.floor (duration / hours) + ':';
- duration = Math.floor (duration % hours);
- }
+ var minutes = Math.floor(duration / one_minute);
+ duration = duration % one_minute;
- /* pad minutes with zeroes of the hours string is non-empty. */
- var minutes_str = hours_str === '' ?
- Math.floor (duration / minutes) + ':' :
- ('00' + Math.floor (duration / minutes)).slice (-2) + ':';
+ var seconds = Math.round(duration / one_second);
- duration = Math.floor (duration % minutes);
+ var ret = '';
+ ret = ('00' + seconds).slice(-2);
- var seconds_str = ('00' + Math.floor (duration / seconds)).slice (-2);
+ if (hours > 0) {
+ ret = hours + ':' + ('00' + minutes).slice(-2) + ':' + ret;
+ }
+ else {
+ ret = minutes + ':' + ret;
+ }
- return hours_str + minutes_str + seconds_str;
+ return ret;
};
Something went wrong with that request. Please try again.