GNOME Bugzilla – Bug 699514
1 hours -> 1 hour
Last modified: 2013-05-15 02:31:53 UTC
gitg will display "1 hours ago" for a commit that's one hour old. This should be "1 hour ago".
Created attachment 243169 [details] [review] Fix for hour relative time. Commits as old as an hour now, read 'An hour ago'. Commits older than an hour now, read 'N hours ago'. Tested with a repository locally. Works correctly. Please review, thank you :)
Review of attachment 243169 [details] [review]: Sindhu, with your change, if a commit is 1 hour and 59 minutes old we'll print "An hour ago". Instead, in that situation we want to print "2 hours ago", rounding to the nearest hour. Please think about this and revise your patch - thanks.
Created attachment 243209 [details] [review] Fix for hour relative time. This patch now checks the difference epoch time like this (t < 3600 * 1.5). Please review, thanks.
Review of attachment 243209 [details] [review]: Sindhu, this looks closer. But let's try an experiment: what will happen in your code if, say, t = 6000? That's between (3600 * 1.5) and (3600 * 2). To find out, we first need to know the type of t. It's declared in the code as a var, but its actual type is the return type of DateTime.to_unix(), which is int64. So let's see: == test.vala == void main() { int64 t = 6000; stdout.puts("%d hours ago\n".printf((int)Math.round(t / 3600)) ); } $ valac test.vala -X -lm $ ./test 1 hours ago $ So this code can still produce "1 hours ago". What's going on here? We've found a bug in the existing code. The author called Math.round() expecting to round to the nearest hour. But since t is a variable of integral type (int64), and 3600 is an integer, the expression (t / 3600) also has an integer type (namely int64). That's just how division works in Vala (or C). It would be different in a language like Python. So Math.round() does nothing here: it's always passed an integer. The expression (t / 3600) performs an integral division, rounding toward zero. So (5000 / 3600) is 1, and we get "1 hours ago". Now, you could decide to try to fix this bug, or not. But in any case I'd suggest a different approach. With your current approach, we have to worry about whether the line return "%d hours ago".printf... might possibly execute with %d = 1. We can stare at the code and use math to convince ourselves that it won't, but that takes effort. I'd instead suggest fixing this bug by checking the value that is being passed to "%d hours ago" - if it's 1, then let's print "hour" rather than "hours". That way you won't need a new condition, and it's safer because it will be obvious from looking at the code that it will never produce "1 hours ago".
(In reply to comment #4) > we have to worry about whether the line > return "%d hours ago".printf... Urgh, that code misses proper I18N anyway. I've filed bug 699635.
Created attachment 243805 [details] [review] Fix for hour relative time Updated patch with (slightly) better logic and hopefully this is more accurate. Please review, thanks!
Review of attachment 243805 [details] [review]: Sindhu, thanks for this latest patch. As I pointed out in my last review, the existing code has a bug: it intends to round to the nearest hour and day by calling Math.round(), but in fact it truncates downward to the nearest hour. So you should be clear about your intention here. Do you intend (a) to fix that bug, so that we actually round to the nearest hour/day? Or (b) to continue truncating, but make a change so that we never print "1 hours"? Assuming that your goal is only (b), the existing code works OK, and only a tiny change is needed to achieve that goal. Here are some questions to think about: 1. In the existing code, exactly which values of t will cause us to print "1 hours"? 2. Can the existing code ever print "1 days ago"? Why or why not? If you answer those questions, I think you'll see a tiny change that will work here. If it's not obvious, ask here again and we can discuss more. I'd prefer a small change rather than rewriting this code since I think the existing code is clear enough. One more point: instead of if (time_in_seconds < (60 * 10)) it's better to write if (time_in_seconds < 60 * 10) (*) has higher precedence than (<) so the extra parentheses are unnecessary and just make the expression harder to read.
Created attachment 243940 [details] [review] Fix for singular and plural relative time formats Patch now addresses the issues: 1. Of rounding to nearest hour. Hours use Math.round to round off epoch difference to nearest hour depending on the mid-point found. Days use Math.floor function. 2. Singular and Plural relative time strings for relative time also works in tandem with ^ 1. Please review. Thank you, Adam for being very patient with me!
Review of attachment 243940 [details] [review]: Sindhu, thanks for your latest patch! And I'm happy to be patient - I taught computer science at a university for three years so I'm happy to explain this stuff. :) So you've decided to fix the rounding bug. OK - that's a reasonable thing to do. Let's see whether your code works. I've pasted it into a program test.vala which runs your code for a couple of different input values. I'll attach test.vala to this bug. You can build it like this: $ valac test.vala -X -lm test.vala runs these tests: void main() { stdout.puts(test(TimeSpan.HOUR + 58 * TimeSpan.MINUTE) + "\n"); stdout.puts(test(TimeSpan.DAY + 22 * TimeSpan.HOUR) + "\n"); } Here we go: $ ./test An hour ago A day ago $ Hm - that's not right. I'll let you ponder why. Here's a hint: when you compute time_in_seconds and time_in_hours, you're performing an integer division, not a floating-point division. This is the same bug that was present in the original sources. I know this will be counterintuitive if you're used to languages like Python. Here are some pages you can read to understand more: http://stackoverflow.com/questions/5456801/c-int-float-casting http://stackoverflow.com/questions/2976011/how-to-get-fractions-in-an-integer-division http://stackoverflow.com/questions/4198626/floating-point-conversion-in-ansi-c Those pages are about C and C++, but this works exactly the same way in Vala. That might not the only bug in your code. When you think you have this fixed, please paste your new code into the test program and run it on various values to be sure it's behaving properly before resubmitting here. One more point. Instead of if (time_in_seconds < 60 * 60 * 24) you could write if (time_in_hours < 24) which is shorter and clearer. You could similarly simplify the test which follows it. Looking forward to your next diff!
Created attachment 243947 [details] test program for previous patch test program for previous patch
Created attachment 243968 [details] [review] Fix for singular and plural relative time formats Updated patch attached. I tested my code with the following snippet: DateTime dt2 = new DateTime.now_local(); dt2 = dt2.add_minutes(-60 * 45); TimeSpan t = (new DateTime.now_local()).difference(dt2); Here, 60 * 45 is now testing for whether our relative time will print "A day ago" or "N days ago". We know that if it exceeds 48 days we can print "2 days ago" but within that we should print "A day ago". Variables where they have been type cased and rounded/floor'ed now make this behaviour happen. I have made the necessary type casts and changes to singular, plural relative time string checks. I have also changed time_in_seconds in If statments to time_in_hours variable where relevant. Please review, again. Thank you :)
Review of attachment 243968 [details] [review]: Sindhu, thanks for this latest patch. I pasted your code into another program test2.vala which I'll attach. It tests your code with these inputs: stdout.printf("%s\n", test(23 * TimeSpan.HOUR + 45 * TimeSpan.MINUTE)); stdout.printf("%s\n", test(TimeSpan.DAY + 22 * TimeSpan.HOUR)); stdout.printf("%s\n", test(6 * TimeSpan.DAY + TimeSpan.HOUR)); The output is this: 24 hours ago A day ago 1 week or longer Some comments: 1. It seems odd that the code sometime prints "24 hours ago" and sometimes "A day ago", since those are two different ways of expressing the same length of time. 2. The second test case above (1 day and 22 hours) prints "A day ago", just like it did in the previous iteration of your patch. As I pointed out last time, that's incorrect: it should be "2 days ago" if our goal is to round to the nearest day. 3. For 6 days and 1 hour we should be printing "6 days ago" if our goal is to round to the nearest day. I'll look forward to the next iteration of your patch. Please explicitly test that all the cases above work correctly before submitting it. Thanks!
Created attachment 243973 [details] test program for previous patch
Created attachment 244152 [details] [review] Fix for singular and plural relative time format. Updated patch based on email discussion with Adam. Please review. Thank you :)
Created attachment 244153 [details] Test program for updated code This is the output for the attached program: =============================================== Time in seconds is 1.000000 Time in hours is 0.000278 10 seconds: A minute ago Time in seconds is 300.000000 Time in hours is 0.083333 5 mins: 5 minutes ago Time in seconds is 1500.000000 Time in hours is 0.416667 25 mins: 25 minutes ago Time in seconds is 2100.000000 Time in hours is 0.583333 35 mins: Half an hour ago Time in seconds is 2280.000000 Time in hours is 0.633333 38 mins: Half an hour ago Time in seconds is 2700.000000 Time in hours is 0.750000 45 mins: An hour ago Time in seconds is 3000.000000 Time in hours is 0.833333 50 mins: An hour ago Time in seconds is 3900.000000 Time in hours is 1.083333 1 hr 5 mins: An hour ago Time in seconds is 5100.000488 Time in hours is 1.416667 1 hr 25 mins: An hour ago Time in seconds is 5699.999512 Time in hours is 1.583333 1 hr 35 mins: 2 hours ago Time in seconds is 7500.000488 Time in hours is 2.083333 2 hr 05 mins: 2 hours ago Time in seconds is 23640.000000 Time in hours is 6.566667 6 hr 34 mins: 7 hours ago
Created attachment 244154 [details] [review] Fix for singular and plural relative time format The previous patch had an error. Attached a properly working patch. Please review. Thank you :)
Review of attachment 244154 [details] [review]: Sindhu, thanks for your latest patch. I think this is the first iteration that you've explicitly tested with various inputs; that's a nice step forward. Of course, it would be nicer if these tests were unit tests inside gitg itself rather than in an external program. But gitg doesn't seem to have any unit tests of this sort today, and I think it would be too large a change to add a unit testing framework to gitg right at this moment, so let's stay with the external tests for now. I modified your test program by adding a few more test cases: stdout.printf("27 minutes: %s\n", test(27 * TimeSpan.MINUTE)); stdout.printf("42 minutes: %s\n", test(42 * TimeSpan.MINUTE)); stdout.printf("23 hr 45 minutes: %s\n", test(23 * TimeSpan.HOUR + 45 * TimeSpan.MINUTE)); stdout.printf("24 hr 15 minutes: %s\n", test(24 * TimeSpan.HOUR + 15 * TimeSpan.MINUTE)); stdout.printf("6 days 1 hours: %s\n", test(6 * TimeSpan.DAY + TimeSpan.HOUR)); Let's see what it prints: 27 minutes: 27 minutes ago 42 minutes: An hour ago 23 hr 45 minutes: 24 hours ago 24 hr 15 minutes: A day ago 6 days 1 hours: More than a week Comments 1 and 3 from my previous review still apply. I'll repeat them here: 1. It seems odd that the code sometimes prints "24 hours ago" and sometimes "A day ago", since those are two different ways of expressing the same length of time. 3. For 6 days and 1 hour we should be printing "6 days ago" if our goal is to round to the nearest day. Furthermore, for 42 minutes it seems wrong to print "An hour ago" since 42 minutes is closer to a half hour than an hour, and in general we're rounding to the nearest printable value.
Created attachment 244167 [details] [review] Fix for singular and plural relative time format Made changes as suggested by Adam. Code no longer prints "24 hours" and "1 day", but only picks "1 day". Please review. Thank you :)
Created attachment 244168 [details] Updated test program Output: ========== 10 seconds: A minute ago 5 mins: 5 minutes ago 25 mins: 25 minutes ago 35 mins: Half an hour ago 38 mins: Half an hour ago 45 mins: An hour ago 50 mins: An hour ago 1 hr 5 mins: An hour ago 1 hr 25 mins: An hour ago 1 hr 35 mins: 2 hours ago 2 hr 05 mins: 2 hours ago 6 hr 34 mins: 7 hours ago 27 minutes: 27 minutes ago 42 minutes: Half an hour ago 23 hr 45 minutes: A day ago 24 hr 15 minutes: A day ago 6 days 1 hours: 6 days ago
Review of attachment 244167 [details] [review]: Sindhu, your latest patch does not contain the same code as your latest test program. For example, the patch includes this: else if (time_in_hours < 24 * 6) But the test program has this line: else if (time_in_hours <= 24 * 7) Of course, this illustrates the danger of having a separate copy of the code for testing rather than proper unit tests. You'll need to be careful to make sure that these copies are in sync. By the way, I strongly recommend looking at your changes in Meld immediately before every commit you ever make, both on this bug and in the future. (If it's not obvious how to do that, email me and we can discuss more.) Let's add one more test case: stdout.printf("23 hours: %s\n", test(23 * TimeSpan.HOUR)); It prints 23 hours: A day ago That's not right: we should print "23 hours ago" here.
Created attachment 244179 [details] [review] Fix for singular and plural relative time format This is the output of test program with this version of code: ============================================================= 10 seconds: A minute ago 5 mins: 5 minutes ago 25 mins: 25 minutes ago 35 mins: Half an hour ago 38 mins: Half an hour ago 45 mins: An hour ago 50 mins: An hour ago 1 hr 5 mins: An hour ago 1 hr 25 mins: An hour ago 1 hr 35 mins: 2 hours ago 2 hr 05 mins: 2 hours ago 6 hr 34 mins: 7 hours ago 27 minutes: 27 minutes ago 42 minutes: Half an hour ago 23 hr 45 minutes: A day ago 23 hr 20 minutes: 23 hours ago 23 hr 30 minutes: 23 hours ago 24 hr 15 minutes: A day ago 6 days 1 hours: 6 days ago
Review of attachment 244179 [details] [review]: OK - this code now handles all cases properly as far as I can tell. One final nit: you've formatted type casts inconsistently in this code. You have a space after the cast here: float time_in_seconds = (float) t / TimeSpan.SECOND; But here you don't: return "%d minutes ago".printf((int)Math.round(time_in_seconds / 60)); You should be consistent. I personally think having a space after the cast makes the code more readable. With this change I think this would be OK to push.
Created attachment 244184 [details] [review] Fix for singular and plural relative time formats Fixed the space issue. Thank you!
Created attachment 244185 [details] [review] Fix for singular and plural relative time formats Previous patch was incorrect!
Pushed to master in 314471d20f43af537122222d4e7c5d278f8f9f91 https://git.gnome.org/browse/gitg/commit/?id=314471d20f43af537122222d4e7c5d278f8f9f91 Closing bug now, thank you so much Adam for all the reviews! :)
Sindhu, I just noticed one edge case I failed to spot in my last review: stdout.printf("1 min 10 sec: %s\n", test(TimeSpan.MINUTE + 10 * TimeSpan.SECOND)); prints 1 min 10 sec: 1 minutes ago We don't ever want to print "1 minutes". Reopening.
Created attachment 244199 [details] [review] Fix for singular minute relative time This is a fresh patch as previous has been pushed to master. Please review. Thanks.
Review of attachment 244199 [details] [review]: Sindhu, even with this patch, the test case I pointed out in my previous review will still print "1 minutes ago". Please test every patch before you attach it to any Bugzilla ticket.
Created attachment 244203 [details] [review] Fix for singular minute relative time My bad sorry, but I got around to it immediately when I noticed it. Updated patch attached.
Review of attachment 244203 [details] [review]: Thanks for this latest patch. Let's try it with a few test cases: stdout.printf("1 min 30 sec: %s\n", test(TimeSpan.MINUTE + 30 * TimeSpan.SECOND)); stdout.printf("2 min 30 sec: %s\n", test(2 * TimeSpan.MINUTE + 30 * TimeSpan.SECOND)); stdout.printf("29 min 45 sec: %s\n", test(29 * TimeSpan.MINUTE + 45 * TimeSpan.SECOND)); stdout.printf("30 min 15 sec: %s\n", test(30 * TimeSpan.MINUTE + 15 * TimeSpan.SECOND)); It prints this: 1 min 30 sec: A minute ago 2 min 30 sec: 3 minutes ago 29 min 45 sec: 30 minutes ago 30 min 15 sec: Half an hour ago Comments: 1. Notice that 1 minute 30 seconds gets rounded down, whereas 2 minutes 30 seconds gets rounded up. That's inconsistent. 2. As these test cases show, we sometimes print "30 minutes ago" and sometimes "half an hour ago". I think that looks strange since these are different ways of expressing the same amount of time. 3. In your code, you handle minutes differently than hours and days. In particular, for minutes you have two if blocks (one for "A minute ago" and another for multiple minutes), but for hours and days you compute a number using rounding and then test whether than number is 1. If you handle minutes the same way as hours and days, the code will be easier to read and understand. That will also make internationalizing this code (bug 699635) easier. Finally, refactoring in this way should automatically fix my point (1) above. In fact, it might also fix point (2) if you first compute a number of minutes and then print "half an hour ago" whenever that number is >= 30.
Created attachment 244224 [details] [review] Fix for minute relative time Updated patch. Updated test program here: http://paste.ubuntu.com/5665317/plain/ Output for the same here: http://paste.ubuntu.com/5665282/plain/ Thanks.
Review of attachment 244224 [details] [review]: I think your code works! :) As a final, optional step you could possibly make your code a little easier to read. Note that instead of if (time_in_seconds < 60 * 29.5) you could just write if (t < 29.5 * TimeSpan.MINUTE) And instead of int rounded_minutes = (int) Math.round(time_in_seconds / 60); you could write int rounded_minutes = (int) Math.round((float) t / TimeSpan.MINUTE); In my opinion both of those would be clearer and more direct. And if you apply similar changes throughout the code that follows, you might not even need the time_in_seconds and time_in_hours variables. :) Up to you.
Created attachment 244235 [details] [review] Better fix for relative time in Gitg Updated patch again. Thank you.
Review of attachment 244235 [details] [review]: Looks good!
Pushed to master in 006943c3238ae1e670cb07d27d9a150eacd15a1f https://git.gnome.org/browse/gitg/commit/?id=006943c3238ae1e670cb07d27d9a150eacd15a1f Closing bug again. Thank you, Adam :)