Created attachment 278133 [details] [review]
Show puzzle completion time as hours, minutes, seconds

examples of how the time will be shown :
- 48 seconds
- 58 minutes, 45 seconds
- 59 minutes
- 1 hour, 21 seconds
- 1 hour, 1 minute, 42 seconds

Review of attachment 278133 [details] [review]:

::: src/sudoku-game.vala
@@ +130,3 @@
     }
+
+    public string get_total_time_string ()

Please make this method static and pure. There is no need for it to be bound to a specific instance and if we can avoid refering to an internal state, we will do that.

(In reply to comment #2)
> Review of attachment 278133 [details] [review]:
> 
> ::: src/sudoku-game.vala
> @@ +130,3 @@
>      }
> +
> +    public string get_total_time_string ()
> 
> Please make this method static and pure. There is no need for it to be bound to
> a specific instance and if we can avoid refering to an internal state, we will
> do that.

Every SudokuGame instance has its own timer.
This method is essentially that timer's 'to_string ()'.
Besides, we're also relying on the game board's previous played time which is set on initialization.
Without it, this would just return an empty string.
I don't understand why there's no need for it to be bound to an instance.

We can have a static function that takes any float/double of seconds worth and converts it to the appropriate string.

that would be
public static string get_time_string (float seconds) { }

It doesn't really need to depend on internal state. We can have it as a pure function that works for every value of seconds we want to pass it.

The call is not much more complicated but the code is now reusable. If we need to do that anywhere else, we can lift the method and have it work.

We generally would want to do that with every data transformation that does not change the state of the instance it is attached to. And it is not a simple getter either. So we can extract the transformation and have the getter as the parameter we pass.

If you want to call to still be instance.get_total_time_string (); you'd want to create the pure function and have 

public string get_total_time_string () {
  return SudokuGame.get_time_string (this.get_total_time_played ());
}

Just for reusabilitie's sake we want to have the pure variant of this method.

Created attachment 278154 [details] [review]
Show puzzle completion time as hours, minutes, seconds

You're right, Mario.

I've made the method static, and renamed it to more generic 'seconds_to_hms_string'.

Review of attachment 278154 [details] [review]:

::: src/sudoku-game.vala
@@ +165,3 @@
+                time_str += ", ";
+            time_str += "%d %s".printf (seconds, second_string);
+        }

I think we can have that even nicer if we use
http://valadoc.org/#!api=glib-2.0/string.joinv

So we would have an array of strings and add the string if we want it and don't add it, if we don't. And then we use joinv to join them with a seperator. Then we wouldn't have the repeated code and we would do away with the time_str variable.

I am sorry for not thinking of that earlier.

Created attachment 278174 [details] [review]
Show puzzle completion time as hours, minutes, seconds

Patch updated to use string.joinv

Review of attachment 278174 [details] [review]:

From a code perspective, I think we're good here and I like it. I haven't tested it but I am sure you did. :)

Comment on attachment 278174 [details] [review]
Show puzzle completion time as hours, minutes, seconds

Pushed as 1515216

Your patch would have been the best approach if we only cared about English, but unfortunately the new strings will be difficult or impossible to localize, so it's time for a quick i18n (internationalization) lesson. There are two separate issues with this patch:

1) Localizing individual strings that are concatenated together is often difficult or impossible. The correct order of the words will be different in different languages, and a single English word may need to be translated differently depending on context. Instead of marking individual strings for translation, we really have to mark only full, completed strings. This will make the code significantly messier and would be bad in an English-only program, but it's not hard to do and is necessary for internationalization.

2) To make this situation even more complicated... different languages have different rules for plurals. In English, 1 is singular and 0, 2, or more are plural.  In other languages (French?), 0 is singular. Other languages have far weirder rules [1]. To solve this, we need to use the function ngettext(). Check out the documentation at [1].

Because life isn't interesting without problems, solving (1) would in this case prevent us from solving (2), and vice versa. (I didn't realize this when creating this bug report.) I'm adding the i18n keyword to this bug in the hopes that someone interested has a grand idea, but I think the way forward with minimal changes is to forget about (1) and fix only (2).

Better options: we could display the time only in minutes (this might be best), or stop tracking it completely (which makes sense because we don't have a pause button). Another idea is to display the time in the window and add a pause button.

One more thing: why was this function added to SudokuGame, if it's only used in gnome-sudoku.vala?

[1] https://www.gnu.org/software/gettext/manual/gettext.html#Plural-forms

Created attachment 278926 [details] [review]
Fix for plural forms

This patch is for plural forms (NOTE I haven't tried to compile it!).
Ideally the order should also be localizable (hms, smh etc.).

Maybe the real solution is to not display this dialog at all, pack a little clock into the end of the header bar, and add a pause button. (We'd then need to draw an overlay over the board when paused; this code could be copied from any game with a clock.) This would bring us in line with the convention for handling clocks in gnome-games.

My suggestion in comment #12 is not a priority and not related to this bug. It's also wrong -- we should not display a timer until we start tracking high scores.

We should instead simply convert the time to minutes, truncate off seconds, and display that.

Created attachment 283372 [details] [review]
Show puzzle completion time in minutes only

(In reply to comment #13)
> My suggestion in comment #12 is not a priority and not related to this bug.
> It's also wrong -- we should not display a timer until we start tracking high
> scores.
> 
> We should instead simply convert the time to minutes, truncate off seconds, and
> display that.

Comment on attachment 283372 [details] [review]
Show puzzle completion time in minutes only

Pushed as 6f9e946

Back once again!

I guess I did not look closely enough at this patch before approving it, sorry. The solution needs to use ngettext.

Created attachment 283693 [details] [review]
Use GLib.ngettext to show puzzle completion time

Review of attachment 283693 [details] [review]:

I think that's right. My track record is not very good in this bug, though. ^^

Comment on attachment 283693 [details] [review]
Use GLib.ngettext to show puzzle completion time

Pushed as ed0612d

FWIW it was wrong :p

See: https://git.gnome.org/browse/gnome-sudoku/commit/?id=c567422e95511ec0835fd05f2ea59ac50858695d

Created attachment 285587 [details] [review]
Fix use of ngettext

At this rate, we may never get this string to work!

Comment on attachment 285587 [details] [review]
Fix use of ngettext

Attachment 285587 [details] pushed as efdd446 - Fix use of ngettext