Note: all the preferences in this dialog should take effect instantly when changed. Settings that require restarting the game (e.g. number of worms) belong in the new game screens instead.

(In reply to Michael Catanzaro from comment #1)
> Settings that require restarting the game (e.g. number of worms) belong in 
> the new game screens instead.

Is enabling/disabling fake bonuses a feature that should take effect immediately?

I think it depends on how we handle scores. In 3.18, games with fake bonuses were scored separately from games without fake bonuses, so it would not make sense for the setting to take effect immediately. That's probably the best way to handle it, but if you're not tracking scores separately then it would make sense for the setting to go in the preferences dialog and take effect immediately.

On the other hand, there might be value in having it take effect immediately simply so that it can go in the preferences dialog instead of trying to shoehorn it into the new game screen.

Created attachment 317754 [details] [review]
Add preferences dialog

Sorry for this late patch, I've been caught up with some other projects too at my university lately.

This preferences dialog I've come up with holds all preferences options of the old Nibbles game except for playing levels in random order, starting level, number of AIs which should belong to the new game screen and relative movement which has not yet been implemented.

Please review and let me know if there is something that needs change.

Review of attachment 317754 [details] [review]:

Beside these minor things, everything looks good. Good job.

::: src/nibbles-game.vala
@@ +534,1 @@
     {

Check if we still have to use this method considering we update the properties as soon as they are changed in the preferences dialog.

@@ +539,3 @@
     }
 
     public void save_properties (Settings settings)

Same as above.

::: src/preferences-dialog.vala
@@ +62,3 @@
+        this.response.connect (() => {
+            this.destroy ();
+            game.load_worm_properties (worm_settings);

Instead of loading properties when the dialog is closed, you should connect a callback to the Settings.changed() signal and load the given key. This way you could also redraw the stage when the worm colors get changed, preventing the current bug where worms change their colors progressively over a few frames.

@@ +83,3 @@
+            radio_button.set_active (speed == game.speed);
+
+            radio_button.toggled.connect (() => {

Use a method instead of a closure/anonymous method as the constructor is turning out to be pretty big.

@@ +86,3 @@
+                if (radio_button.active)
+                {
+                    settings.set_int ("speed", speed);

Better add a method in the game class that receives a key and a value, and updates the settings file.

@@ +87,3 @@
+                {
+                    settings.set_int ("speed", speed);
+                    game.speed = speed;

Instead of manually changing two things each time a setting is changed, you could connect to the Settings.changed signal which has the changed key as the first argument.

@@ +97,3 @@
+            var play_sound = sound_check_button.active;
+            settings.set_boolean ("sound", play_sound);
+            view.is_muted = !play_sound;

Same as above.

@@ +103,3 @@
+        fakes_check_button.set_active (settings.get_boolean ("fakes"));
+        fakes_check_button.toggled.connect (() => {
+            var are_fakes = fakes_check_button.active;

Better use "has_fakes" or just "fakes"

@@ +105,3 @@
+            var are_fakes = fakes_check_button.active;
+            settings.set_boolean ("fakes", are_fakes);
+            game.fakes = are_fakes;

Same as above, use the signal and use a method

Created attachment 317801 [details] [review]
[Updated] Add preferences dialog

I made some changes as you suggested: got rid of those closures and replaced them with callbacks and also discarded that load_worm_properties call for a new callback is now connected to the settings.changed() signal.

Yet I'm afraid the methods for loading and saving settings have to stay as they are also used at game startup and shutdown.

Also, the game class lacks a settings field so I strongly believe that there is no point in adding one just for updating the settings file.

Looking forward to your feedback.

Review of attachment 317801 [details] [review]:

I think it's better now. You only need to fix these minor things related to translation I can commit it.

::: src/preferences-dialog.vala
@@ +74,3 @@
+        radio_buttons.add (medium_radio_button);
+        radio_buttons.add (slow_radio_button);
+        radio_buttons.add (beginner_radio_button);

You have added these buttons in the wrong order; beginner should be at the top, followed by slow, medium, fast. It's ok everywhere else in the code :)

@@ +84,3 @@
+
+        /* Sound check button */
+        sound_check_button.set_active (settings.get_boolean ("sound"));

You should either use the value stored in game (as you did for speed) or use the one stored in settings (as you did here and below, for sound), but not both :)

@@ +88,3 @@
+
+        /* Fake bonuses check button */
+        fakes_check_button.set_active (settings.get_boolean ("fakes"));

Ditto.

@@ +124,3 @@
+
+            var label_renderer = new Gtk.CellRendererText ();
+            tree_view.insert_column_with_attributes (-1, "Action", label_renderer, "text", 1);

Make "Action" translatable.

@@ +131,3 @@
+            key_renderer.accel_edited.connect (accel_edited_cb);
+            key_renderer.accel_cleared.connect (accel_cleared_cb);
+            tree_view.insert_column_with_attributes (-1, "Key", key_renderer, "accel-key", 2);

"Key" should be translatable as well.

@@ +157,3 @@
+    private void radio_button_toggled_cb (Gtk.ToggleButton button)
+    {
+        if (button.active)

Change this to "get_active()" to maintain consistency.

@@ +166,3 @@
+    private void sound_toggled_cb ()
+    {
+        var play_sound = sound_check_button.active;

get_active()

@@ +172,3 @@
+    private void fakes_toggles_cb ()
+    {
+        var has_fakes = fakes_check_button.active;

get_active()

@@ +213,3 @@
+                                                    Gtk.MessageType.WARNING,
+                                                    Gtk.ButtonsType.OK,
+                                                    "The key you selected is already assigned!");

Make this string translatable as well. Also, you should add a comment for the translators.

Created attachment 317967 [details] [review]
[Update #2] Add preferences dialog

I've done as you instructed and got rid of the game object in PreferencesDialog class (now only uses values stored in settings).

Also added mnemonics and translatable strings.

Review of attachment 317967 [details] [review]:

:)

Review of attachment 317967 [details] [review]:

:)