Created attachment 283696 [details] [review]
Patch

Apparently, the patch didn't get attached in the original post. :/

Comment on attachment 283696 [details] [review]
Patch

This is a binary file :/   Some sort of vim history thing?

Created attachment 283715 [details] [review]
Correct patch (hopefully!)

After getting it wrong twice, I hope this is the correct one.

I was probably too sleepy yesterday, sorry. :(

Review of attachment 283715 [details] [review]:

The only major problem I see is that when I finish a game of Robots but do NOT get a high score, the scores dialog should not be opened. (Sorry for not noticing that earlier.) The rest of these comments are minor:

::: src/game.c
@@ +94,3 @@
 /**********************************************************************/
 
+GamesScoresCategory* current_cat;

Notice that the declarations at the top of this file are organized into exported variables, file static variables, and function prototypes. My first thought was that you should move this up to live with the other exported variables, but actually this variable should not be exported at all: it's not needed in other files, and you did not declare it in game.h. So instead, mark it as static to prevent it from being exported, and move it up to the file static variables section.

Also, the * goes one space away from the data type in GNOME code.  The opposite style is not uncommon, and actually more common in C++, but the GNOME style is more typical in C:

static GamesScoresCategory *current_cat;

@@ +131,3 @@
 {
+  games_scores_context_run_dialog (highscores);
+  return 0;

This part of the diff looks like a big win for your scores library. I like it when we can delete code from the games that doesn't really need to be different between them.

Be sure to update the parameters (should be void) and also the return value (should also be void). You'll need to do this in game.h as well, and change each place this function is called.

@@ +165,3 @@
   if (sc != 0) {
+    const gchar* key = sbuf;
+    const gchar* name = category_name_from_key (key);

The *s should be on the variable name, not the data type.  But I think you can remove both of these by doing current_cat = games_scores_category_new (sbuf, category_name_from_key (sbuf))

However, I'm worried that you return a gchar * from category_name_from_key(), and assign it here to a const gchar *.  In GNOME we use the constness to indicate whether the string needs to be freed, so this would normally be a memory leak.  In this case you just got the return type wrong: category_name_from_key() should be declared to return a const gchar * since it returns a sting that's declared statically.

Actually, you declared it to return a gchar * in the header file, but defined it you return a const gchar *.  Better fix this.

::: src/gnome-robots.c
@@ +1,1 @@
+

Extra blank line added at the start of this file

@@ +119,3 @@
   {"robots_with_safe_teleport-super-safe", N_("Robots with safe teleport with super-safe moves")}
 };
+static gint no_categories = 15;

Instead of hardcoding this, you should compute it at runtime so that your code doesn't break if you forget to update it: sizeof (scorecats) / sizeof (scorecats[0]). Anyway we have a macro to make this nicer: just do G_NUM_ELEMENTS (scorecats).

Fair warning for the future: this only works where the size of the array is known. If you pass it to a function or declare it on the heap, then sizeof (key_value_scorecats) would be the same as sizeof (key_value_scorecats *).

@@ +174,3 @@
 
+const gchar
+*category_name_from_key (const gchar* key)

const gchar *
category_name_from_key (const gchar *key)

@@ +182,3 @@
+      return scorecats[i].name;
+  }
+  /*Return key as is if match not found*/

This comment is wrong.  I would just remove it, since it's pretty clear how this works.

@@ +336,3 @@
 
+static GamesScoresCategory
+*create_category_from_key (GamesScoresContext *context, const char *key, gpointer user_data)

static GamesScoresCategory *
create_category_from_key (...)

Here you got the spacing in the parameter list correct!

@@ +453,3 @@
 					  ("The program Robots was unable to find any valid game configuration files. Please check that the program is installed correctly."));
     gtk_window_set_resizable (GTK_WINDOW (errordialog), FALSE);
+

Added an extra blank line here

::: src/gnome-robots.h
@@ +21,3 @@
 void update_game_status (gint score, gint level, gint safe_teleports);
 void quit_game (void);
+gchar* category_name_from_key (const gchar*);

This is my fault since I suggested this function name yesterday, but can you please rename it to scores_category_name_from_key() to be even more descriptive.  If it was a static function then the current shorter name would arguably be better, but when the scope of a name increases it becomes more important to use a more specific name.

Also, * goes one space away from the data type:

const gchar *category_name_from_key (const gchar *);

(In reply to comment #4)
> Instead of hardcoding this, you should compute it at runtime so that your code
> doesn't break if you forget to update it: sizeof (scorecats) / sizeof
> (scorecats[0]). Anyway we have a macro to make this nicer: just do
> G_NUM_ELEMENTS (scorecats).

Whoops, the macro is actually G_N_ELEMENTS!

Created attachment 283812 [details] [review]
Patch

This patch hopefully takes care of all the issues.

Review of attachment 283812 [details] [review]:

Good, this is nearly done.

::: src/game.c
@@ +159,3 @@
   }
 
+  gint result = GTK_RESPONSE_ACCEPT;

So we might start a new game even if the dialog is never displayed (if sc == 0)?

@@ +165,3 @@
+    static GtkWidget *sorrydialog = NULL;
+    GError *error = NULL;
+    if (!games_scores_context_add_score (highscores, (guint32) sc, current_cat, &error))

The parameter is a glong so you should cast it to a glong, not a guint32

Also if you're going to ignore errors you should just pass NULL, as this way you leak the GError if there is an error. Don't do that though: check if error != NULL and if so, log a warning with g_warning() and then free the error with g_error_free()

@@ +166,3 @@
+    GError *error = NULL;
+    if (!games_scores_context_add_score (highscores, (guint32) sc, current_cat, &error))
+    {

Brace goes on the end of the previous line

@@ +186,3 @@
   g_free (sbuf);
 
+  return result;

Hm, I don't like that we're returning a dialog response from this function. I'd rather move the calls to quit_game() and start_new_game() into this function, and then rename it from log_score() to add_score().

::: src/game.h
@@ +27,3 @@
 void quit_game (void);
 void game_keypress (gint);
+void show_scores ();

In C++ this would be correct, but in C this is a declaration for a function that takes an unlimited number of parameters: the void is mandatory.

::: src/gnome-robots.c
@@ +119,3 @@
 };
 
+static gint no_categories = G_N_ELEMENTS (scorecats);

You should get rid of no_categories and use G_N_ELEMENTS directly

::: src/gnome-robots.h
@@ +21,3 @@
 void update_game_status (gint score, gint level, gint safe_teleports);
 void quit_game (void);
+const gchar *scores_category_name_from_key (const gchar*);

... (const gchar *);

with a space before that last *

Created attachment 283825 [details] [review]
Patch

Improvements based on last patch review.

Review of attachment 283825 [details] [review]:

So close :)

::: src/game.c
@@ +126,3 @@
 
+void
+show_scores ()

void ;)

Created attachment 283828 [details] [review]
Patch

Please god, let this be the last one. :D

Review of attachment 283828 [details] [review]:

Good

ping - Patch here is set to "accepted-commit_after_freeze" and might be nice to get it in before it is again blocke by a freeze.

Comment on attachment 283828 [details] [review]
Patch

Poor choice of patch status on my part; the patch is good from a GNOME Robots perspective but we need to get libgames-scores into gnome-apps and update other games. This will probably happen after the next freeze.