Created attachment 285651 [details] [review]
Warning user when changing board size

Review of attachment 285651 [details] [review]:

::: src/gnome-tetravex.vala
@@ +476,3 @@
+        {
+            action.change_state (parameter);
+        }

I think this all should happen in size_changed ( ... ) (~:429) and not in the general radio_cb.

@@ +479,3 @@
+        else
+        {
+            var size = ((string) parameter)[0] - '0';

This is taken straight from size_changed(). This is the hint that the code actually belongs there.

::: src/puzzle-view.vala
@@ +106,3 @@
     }
+
+    public bool has_tile_on_left ()

This should be something more like "game is underway" and should also return false if the game is finished (you can use is_solved for that).

Created attachment 285667 [details] [review]
Warning user when changing board size

Review of attachment 285667 [details] [review]:

I think we're getting there. :)

::: src/gnome-tetravex.vala
@@ +441,3 @@
+                                                _("Are you sure you want to select new board size?"));
+            dialog.add_buttons (_("_Keep Playing"), Gtk.ResponseType.REJECT,
+                                                Gtk.DialogFlags.MODAL,

These texts may need changing by a native speaker. I would opt for:
"Changing the size of the board starts a new game and the current progress will be lost.".
And "Cancel" and "Continue".

Because that was the issue, that the game restarts unexpectedly. Therefore we want to inform the user that this is the case.

@@ +452,3 @@
+                game_size = (int) size;
+                action.set_state (value);
+                                null);

I think this code doesn't need to be duplicated. We can just have that at the end of the method and return earlier, as we do in line 434.

So maybe something like if response != accept -> return and removing the else-part of the if-statement.

The thing is, we actually always want to do those four lines (changing settings and starting a new game) but there are cases where we just return early. The code shoud reflect that.

::: src/puzzle-view.vala
@@ +106,3 @@
     }
+
+    public bool game_in_progress ()

I think we can have that a read-only-property. This is pretty nice in vala.

@@ +110,3 @@
+        if (!puzzle.is_solved)
+        {
+            for (uint y = 0; y < puzzle.size; y++)

Can we not nest this too much? We can start this method off with if (!puzzle.is_solved) return false;

This doesn't change anything for the result but increases readability. We foremost write code for humans/developers and not for the compiler. So obviousness and readability are always key.

(In reply to comment #4)
> Review of attachment 285667 [details] [review]:
> 
> I think we're getting there. :)
> 
> ::: src/gnome-tetravex.vala
> @@ +441,3 @@
> +                                                _("Are you sure you want to
> select new board size?"));
> +            dialog.add_buttons (_("_Keep Playing"), Gtk.ResponseType.REJECT,
> +                                                Gtk.DialogFlags.MODAL,
> 
> These texts may need changing by a native speaker. I would opt for:
> "Changing the size of the board starts a new game and the current progress will
> be lost.".
> And "Cancel" and "Continue".

How about:

_("Are you sure you want to start a new game with a different board size?")

_("_Keep Playing")
_("Start _New Game")

Created attachment 285678 [details] [review]
Warning user when changing board size

Review of attachment 285678 [details] [review]:

::: src/puzzle-view.vala
@@ +106,3 @@
     }
+
+    public bool game_in_progress

This should be a function, is_game_in_progress, not a property.

@@ +118,3 @@
+                {
+                    if (puzzle.get_tile (x, y) == null)
+                    {

No need for the extra braces on the if statement; they don't make the code easier to read.

(In reply to comment #4)
> ::: src/puzzle-view.vala
> @@ +106,3 @@
>      }
> +
> +    public bool game_in_progress ()
> 
> I think we can have that a read-only-property. This is pretty nice in vala.

Eh, sorry, follow Mario's preference here, not mine.  I like to avoid complex properties but that's just a matter of style; either way is fine.

I suggested that because since we *do* have properties in our language. Methods, even without parameters, maybe imply side effects for the reader. :)

reading a property should not have side effects

Created attachment 285680 [details] [review]
Warning user when changing board size

Review of attachment 285680 [details] [review]:

Just one problem left:

::: src/gnome-tetravex.vala
@@ +433,3 @@
         if (size == settings.get_int (KEY_GRID_SIZE))
             return;
+        if (view.game_in_progress ())

Does this compile?  :)

Created attachment 285838 [details] [review]
Warning user when changing board size

Review of attachment 285838 [details] [review]:

Looks good.

Thanks . :)