After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 754608 - Merge wip/vala into master
Merge wip/vala into master
Status: RESOLVED FIXED
Product: gnome-nibbles
Classification: Applications
Component: general
git master
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-nibbles-maint
gnome-nibbles-maint
Depends on:
Blocks:
 
 
Reported: 2015-09-05 17:39 UTC by Michael Catanzaro
Modified: 2015-11-21 18:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wip/ai in one patch (883.26 KB, patch)
2015-09-05 17:39 UTC, Michael Catanzaro
needs-work Details | Review
Screenshot of opening screen color scheme (15.34 KB, image/png)
2015-09-05 19:42 UTC, Michael Catanzaro
  Details
Screenshot of problem on controls screen (12.25 KB, image/png)
2015-09-05 19:42 UTC, Michael Catanzaro
  Details
Patch for comment 5 (1.87 KB, patch)
2015-09-17 14:15 UTC, Razvan Chitu
reviewed Details | Review
Updated patch for comment 5 (1.88 KB, patch)
2015-09-17 19:24 UTC, Razvan Chitu
reviewed Details | Review
Patch for comment 6 (1.33 KB, patch)
2015-09-17 19:29 UTC, Razvan Chitu
none Details | Review
Updated patch for comment 5 (1.88 KB, patch)
2015-09-18 10:50 UTC, Razvan Chitu
committed Details | Review
Updated patch for comment 6 (1.33 KB, patch)
2015-09-18 10:51 UTC, Razvan Chitu
committed Details | Review
Patch for comment 8 (6.98 KB, patch)
2015-09-18 14:26 UTC, Razvan Chitu
none Details | Review
Updated patch for comment 8 (6.98 KB, patch)
2015-09-21 11:13 UTC, Razvan Chitu
committed Details | Review
Patch for respawn delay (4.07 KB, patch)
2015-09-30 16:35 UTC, Razvan Chitu
committed Details | Review
Move teensy classes, structs and enums at the top (7.96 KB, patch)
2015-10-25 19:49 UTC, Gabriel Ivașcu
committed Details | Review
Remove unused method in NibblesGame class (1.20 KB, patch)
2015-10-26 20:16 UTC, Gabriel Ivașcu
committed Details | Review

Description Michael Catanzaro 2015-09-05 17:39:27 UTC
Bug to facilitate code review, with a patch where all of Iulian's commits are merged into one commit.
Comment 1 Michael Catanzaro 2015-09-05 17:39:52 UTC
Created attachment 310711 [details] [review]
wip/ai in one patch
Comment 2 Michael Catanzaro 2015-09-05 19:40:01 UTC
Review of attachment 310711 [details] [review]:

This turned out REALLY well; good job. :) It's the same classic Nibbles with all the same levels, but modernized with great art from Allan. And the code is, for the most part, a breeze to review.

There are some things that need to be fixed still, of course! Here, I played a game through several levels until I died, leaving my impressions. Next, I review all the code changes, except nibbles-view.vala, which I haven't gotten to yet. I am still on the hook to review nibbles-view.vala, but I think you have enough comments to work through for now. :) You can ignore the few comments I left on GitHub; I've either included them here or fixed them myself.

Lastly, I plan to simply 'git rebase' (or maybe 'git merge') your entire wip/ai branch onto master so as to keep your history, rather than applying this all as a single commit. Attaching it all as one patch here is just to make reviewing easier.

Here we go:

The color scheme on the opening screens are rather messed up; the text almost blends into the background, and the characters on the keyboard keys blend into the keys. Maybe it looks better in the dark theme than in the default theme? (Ideally, it wouldn't use theme colors at all.) It might help to request the dark theme. I think the designs have Nibbles using the dark theme, right? If so, it needs to request it from GTK+. Example: https://git.gnome.org/browse/quadrapassel/commit/src?id=045ecffabcc118f320fd52cc3bf3b429131b96ed

Similarly, the difference between text color when the window is focused and unfocused is too dramatic. I would pick the current unfocused colors, which look much better, and use them always.

Let's remove New Game and Pause from the app menu. They're not needed there, now that they have buttons in the window, which is a better place for them.

I guess it's not possible to go over 6 lives anymore? That's fine I guess (especially since there's not really room for them), though it used to be fun racking up extra lives. Hm, actually, it looks like I do keep the extra lives, they're just not displayed. That's not good: too confusing.

The transition from one level to the Level Completed! screen is a bit off. It's good enough for now; just needs a bit of polish....

I wonder what I need to do in jhbuild to make the sounds work....

I think there is some bug with Pause, but I can't reproduce it. The game unpaused itself shortly after I clicked pause, and all the worms were running again (I almost died! :) but the Resume label was still visible. Also something we can deal with later, if you can't figure it out right away.

The worms start moving at the start of each level before keyboard input is available: there is some delay before I have control of my worm after it starts moving. That's quite bad! It might not be related, but such issues are sometimes caused by using Timeout.add_seconds (which is usually preferable, since it saves power, but is imprecise!) instead of Timeout.add.

Teleporter death bugs seem to be nicely resolved in your rewrite. :)

Also need:

* Preferences dialog
* AI worms by default; I found I had to change NibblesGame.numai manually to get AI
* The "orange" worm is definitely red; needs to be a different color
* Consider the default game speed; feels just a little too fast, but up to you
* Need to fix libgames-scores to not crash immediately ;p
* Updated user help (mostly just new screenshots)
* Ideally, fix the FIXMEs

::: configure.ac
@@ +29,1 @@
 PKG_CHECK_MODULES(GNOME_NIBBLES, [

I like to alphabetize these, but you don't have to.

Also, I notice that the min required versions of clutter and clutter-gtk are lower than they were before. That's plausible but a bit odd; I guess you lowered them because you checked.

::: src/nibbles-game.vala
@@ +23,3 @@
+{
+    public int tile_size;
+    public int start_level;

Make these properties or else private.

@@ +46,3 @@
+
+    public int current_level;
+    public int[,] board;

Ditto, for this huge run of public fields

@@ +74,3 @@
+    public signal void level_completed ();
+
+    public Gee.HashMap<Worm, WormProperties?> worm_props;

Ditto

@@ +156,3 @@
+        {
+            end ();
+            var winner = get_winner ();

Nit of all nits, but the line spacing looks a bit odd here. I would leave a blank line after end(), like you did above. Winner is more closely related to the conditional below it (where you did leave a blank line; I might not have) than it is to end().

@@ +583,3 @@
+}
+
+public enum GameStatus

Please put this at the top of the file, instead of the bottom.

::: src/warp.vala
@@ +20,3 @@
+// Sean MacIsaac, Ian Peters, Guillaume Béland.
+
+public class Warp : Object

Stupid Vala prevents us from using a struct here, correct? Then I would use properties instead of public fields. That way you can make them public get, private set.

@@ +26,3 @@
+
+    public int wx;
+    public int wy;

Let's take the opportunity to clean this up. What is wx/wy? I guess it's the coordinates that will be warped to. A more descriptive name would be ideal (target_x, target_y) but if that would be cumbersome, at least leave a comment.

@@ +38,3 @@
+}
+
+public class WarpManager: Object

Leave a space before the colon.

@@ +42,3 @@
+    private const int MAX_WARPS = 200;
+
+    public Gee.LinkedList<Warp> warps;

Should be private, or a property.

@@ +62,3 @@
+                {
+                    warp.wx = wx;
+                    warp.wy = wy;

Er, what is this doing? If x < 0 and another teleporter points to the same row, change the coordinates of the other teleporter? Let's either reconsider if this is needed, or add a comment to explain what is going on. If you don't understand it either, then the former. :)

@@ +80,3 @@
+                    warp.x = x;
+                    warp.y = y;
+                    add = false;

Ditto

::: src/worm.vala
@@ +26,3 @@
+    public const int MAX_LIVES = 12;
+
+    public const int GROW_FACTOR = 4;

All of these constants should be static, since each worm does not need its own copy: they can't differ for each worm.

@@ +34,3 @@
+    public bool is_human;
+    public bool keypress = false;
+    public bool is_stopped = false;

These should be properties or private.

@@ +37,3 @@
+
+    public int lives { get; set; }
+    public int change;

Ditto. Also, change should have a comment to indicate how it works, since it's not immediately clear. (I guess it is for storing the number of new segments left to be added.)

@@ +43,3 @@
+    {
+        get { return list.size; }
+        set {}

Does simply omitting the set not disallow it?

@@ +51,3 @@
+        {
+            Position head = list.first ();
+            return head;

Can this not be a one-liner?

@@ +59,3 @@
+    }
+
+    public WormDirection direction;

property or private

@@ +61,3 @@
+    public WormDirection direction;
+
+    public WormDirection starting_direction;

property or private

@@ +133,3 @@
+                break;
+            default:
+                break;

Call assert_not_reached () on the line before the break.

@@ +150,3 @@
+        {
+            board[list.last ().x, list.last ().y] = NibblesGame.EMPTYCHAR;
+            list.poll_tail ();

Yeah, a simple linked list worked very well here. This nice code will help us avoid many bugs in the future.

@@ +311,3 @@
+            dir = (WormDirection) 1;
+        if (dir < 1)
+            dir = (WormDirection) 4;

I think it's fine to assume the function will never be called with an invalid WormDirection. That's the value of using enums: to enforce what values are legal and what aren't.

@@ +389,3 @@
+
+    public void dequeue_keypress ()
+                requires (!key_queue.is_empty)

This is an awful lot of indentation. I would use only four additional spaces here:

public void dequeue_keypress ()
    requires (!key_queue.is_empty)

@@ +395,3 @@
+
+    /*\
+    * * AI

It's generally nice to have more small files than a few large one. The AI section looks like a possible candidate to be split off. But you might find more value in keeping it here where it has access to the private members of Worm.

I'm going to skim over the AI, by the way, since it seems like a very literal translation from the original code: it can't be any worse than it was before, and it is making my head hurt. :)

@@ +778,3 @@
+}
+
+public struct WormProperties

Position, WormDirection, and WormProperties should either be declared at the top of the file, above Worm, or inside Worm (also up at the top). Logically, you want to know what they are before reading the rest of the file, not after.
Comment 3 Michael Catanzaro 2015-09-05 19:42:30 UTC
Created attachment 310719 [details]
Screenshot of opening screen color scheme

This has been a problem for a month or two now; I just didn't bug you about it yet.
Comment 4 Michael Catanzaro 2015-09-05 19:42:50 UTC
Created attachment 310720 [details]
Screenshot of problem on controls screen
Comment 5 Michael Catanzaro 2015-09-05 19:44:36 UTC
I think the title of the header bar while the game is in progress should be "Level n" rather than just Nibbles. We know it's Nibbles. :)
Comment 6 Michael Catanzaro 2015-09-05 19:46:05 UTC
Clicking New Game should not unpause the game.
Comment 7 Michael Catanzaro 2015-09-05 19:55:16 UTC
(In reply to Michael Catanzaro from comment #2)
> * Consider the default game speed; feels just a little too fast, but up to
> you

I'm starting to like it. Certainly more fun than the slower default speed we used before.
Comment 8 Michael Catanzaro 2015-09-05 19:57:24 UTC
If I resize the window (or maximize/unmaximize) while the game is playing, the position of all the bonuses gets messed up.
Comment 9 Michael Catanzaro 2015-09-05 20:11:43 UTC
Can we add a brief grace period between a worm's death and when he starts moving again? It's a bit brutal otherwise.

Also, what happens if a worm dies while another worm is on its spawn area? Does the unfortunate non-dead worm die too? I think that is the behavior in the original game. I don't know if that's expected or not. :)
Comment 10 Michael Catanzaro 2015-09-06 00:40:42 UTC
Review of attachment 310711 [details] [review]:

In nibbles-view.vala, I'd again rather put the teensy classes at the top of the file than the bottom. Normally I like to have only one class per file, but for these teensy classes, it's pretty hard to justify giving them their own files.

::: src/nibbles-view.vala
@@ +20,3 @@
+{
+    /* Sound */
+    public bool is_muted;

You already know what I will say about this :)

@@ +59,3 @@
+
+    /* Colors */
+    public const int NUM_COLORS = 6;

static

@@ +95,3 @@
+    {
+        string level_name;
+        string filename;

I prefer to declare variables with the smallest scope that is reasonable: typically the line where they are first used. You don't have to put them all at the top of the function because this isn't C89. :)

@@ +100,3 @@
+
+        level_name = "level%03d.gnl".printf (level);
+        filename = Path.build_filename (PKGDATADIR, "levels", level_name, null);

Did you consider putting these into the gresource?

@@ +331,3 @@
+    public Gdk.Pixbuf load_pixmap_file (string pixmap, int xsize, int ysize)
+    {
+        var filename = Path.build_filename (PKGDATADIR, "pixmaps", pixmap, null);

Same here. The value of putting these into the gresource is that there is no I/O that can fail.

@@ +335,3 @@
+        {
+            /* Fatal console error when the game's data files are missing. */
+            error (_("Nibbles couldn't find pixmap file: %s"), filename);

Ah, at long last, after so many trivial review comments, I have finally found a real mistake in your code! Path.build_filename() does not check to see if the file exists on disk; it just concatenates all its arguments into a file path. This error path will never be reached.

The return value is not nullable either, so it's a shame that the compiler even permitted the comparison against null. :/
Comment 11 Michael Catanzaro 2015-09-06 00:46:28 UTC
It seems quite stable overall, but I eventually found a crash:

ERROR:/home/mcatanzaro/jhbuild/checkout/libgee/gee/linkedlist.vala:299:gee_linked_list_last: assertion failed: (_size > 0)

Program terminated with signal SIGABRT, Aborted.
  • #0 __GI_raise
    at ../sysdeps/unix/sysv/linux/raise.c line 55
  • #0 __GI_raise
    at ../sysdeps/unix/sysv/linux/raise.c line 55
  • #1 __GI_abort
    at abort.c line 89
  • #2 g_assertion_message
  • #3 g_assertion_message_expr
    at gtestutils.c line 2444
  • #4 gee_linked_list_last
    at /home/mcatanzaro/jhbuild/checkout/libgee/gee/linkedlist.vala line 299
  • #5 worm_reduce_tail
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/worm.vala line 178
  • #6 nibbles_game_apply_bonus
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/nibbles-game.vala line 427
  • #7 nibbles_game_bonus_found_cb
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/nibbles-game.vala line 452
  • #8 _nibbles_game_bonus_found_cb_worm_bonus_found
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/nibbles-game.vala line 196
  • #9 g_cclosure_marshal_VOID__VOIDv
    at gmarshal.c line 905
  • #10 _g_closure_invoke_va
    at gclosure.c line 864
  • #11 g_signal_emit_valist
    at gsignal.c line 3281
  • #12 g_signal_emit_by_name
  • #13 worm_move
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/worm.vala line 158
  • #14 nibbles_game_move_worms
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/nibbles-game.vala line 284
  • #15 nibbles_game_main_loop_cb
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/nibbles-game.vala line 179
  • #16 _nibbles_game_main_loop_cb_gsource_func
    at nibbles-game.c line 404
  • #17 g_timeout_dispatch
    at gmain.c line 4577
  • #18 g_main_dispatch
    at gmain.c line 3154
  • #19 g_main_context_dispatch
    at gmain.c line 3769
  • #20 g_main_context_iterate
    at gmain.c line 3840
  • #21 g_main_context_iteration
    at gmain.c line 3901
  • #22 g_application_run
    at gapplication.c line 2311
  • #23 nibbles_main
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/gnome-nibbles.vala line 786
  • #24 main
    at /home/mcatanzaro/jhbuild/checkout/gnome-nibbles/src/gnome-nibbles.vala line 748

Comment 12 Michael Catanzaro 2015-09-13 17:52:44 UTC
(In reply to Michael Catanzaro from comment #9) 
> Also, what happens if a worm dies while another worm is on its spawn area?
> Does the unfortunate non-dead worm die too? I think that is the behavior in
> the original game. I don't know if that's expected or not. :)

The original behavior is to crash: bug #735029

Let's not reproduce that. :D
Comment 13 Razvan Chitu 2015-09-17 14:15:42 UTC
Created attachment 311557 [details] [review]
Patch for comment 5

(In reply to Michael Catanzaro from comment #5)
> I think the title of the header bar while the game is in progress should be
> "Level n" rather than just Nibbles. We know it's Nibbles. :)

Hello, I'm a student interested in the possibility of contributing to open-source software. I've heard about GNOME from friends who have contributed (mostly from Iulian) and it got me curious. So, here is my first patch.
Comment 14 Michael Catanzaro 2015-09-17 14:59:18 UTC
Review of attachment 311557 [details] [review]:

Thanks for your patch. It looks good; I trust Iulian will take care of merging this into his branch, or object if he doesn't like it.

::: src/gnome-nibbles.vala
@@ +471,3 @@
         back_action.set_enabled (false);
 
+        headerbar.set_title (_(@"Level $(game.current_level)"));

I think using a string template for a translatable string is a bit much for the translators. We should use a printf here instead:

_("Level %d".printf(game.current_level))

The reason being that translators are used to seeing format strings, and much less likely to break it. :)
Comment 15 Michael Catanzaro 2015-09-17 15:00:22 UTC
I think I got that wrong; it should probably be:

_("Level %d").printf(game.current_level)
Comment 16 Razvan Chitu 2015-09-17 19:24:07 UTC
Created attachment 311582 [details] [review]
Updated patch for comment 5

(In reply to Michael Catanzaro from comment #15)
> I think I got that wrong; it should probably be:
> 
> _("Level %d").printf(game.current_level)

Updated patch attached.
Comment 17 Razvan Chitu 2015-09-17 19:29:45 UTC
Created attachment 311583 [details] [review]
Patch for comment 6

(In reply to Michael Catanzaro from comment #6)
> Clicking New Game should not unpause the game.

New game button no longer unpauses the game, only stops it if it's running. I also noticed that when using the escape key to close the dialog, a different event is fired and the game would not start, so it's been added to the `if` condition.
Comment 18 Michael Catanzaro 2015-09-17 20:54:56 UTC
Looks fine... I will let Iulian review these, though.

By the way, please do set your email so it shows up properly, using 'git config --global user.email youremailhere@whatever.com'. That winds up in the patches you post and is used for commit authorship.
Comment 19 Razvan Chitu 2015-09-18 10:50:43 UTC
Created attachment 311624 [details] [review]
Updated patch for comment 5
Comment 20 Razvan Chitu 2015-09-18 10:51:34 UTC
Created attachment 311625 [details] [review]
Updated patch for comment 6
Comment 21 Iulian Radu 2015-09-18 10:52:22 UTC
Review of attachment 311624 [details] [review]:

Looks good!
Comment 22 Iulian Radu 2015-09-18 10:55:11 UTC
Review of attachment 311625 [details] [review]:

Good job!
Comment 23 Razvan Chitu 2015-09-18 14:26:00 UTC
Created attachment 311637 [details] [review]
Patch for comment 8

(In reply to Michael Catanzaro from comment #8)
> If I resize the window (or maximize/unmaximize) while the game is playing,
> the position of all the bonuses gets messed up.

In this patch, I:

- changed warps to be children of level instead of children of stage (discussed it with Iulian and it makes more sense to be so). Because of this, the position of warps is set in the board_rescale method, so it does not need to be set in warps_rescale;
- removed position setting from boni_rescale because it was done in board_rescale;
- defined new set_size methods for BonusTexture and WarpTexture to make use of a size multiplication constant, so set_size is not called with `2 * tile_size`.
Comment 24 Michael Catanzaro 2015-09-20 17:46:11 UTC
Turns out that string templates are untranslatable; it's implemented as a series of g_strconcats. I will change the existing ones into printfs.

Also, thinking a bit more about command line warnings. Originally I was not sure if these should be translated or not. Thinking about it more, the result of a GError is usually not going to be translated, and there is little value is showing a half-translated string, and these aren't really user visible anyway. So I think we should not be translating these. I will change these too.

Also, I fixed some things in the libgames-scores API. I will update Nibbles.

Also, I'm adding some FIXMEs for how we handle the end of the game.
Comment 25 Michael Catanzaro 2015-09-20 18:19:26 UTC
Some comments on what I'm about to push.

It's better to avoid putting markup in the translatable strings, when possible. "Player %d" is much harder for a translator to break than "<b>Player %d</b>" (and much much harder to break than the string template, presuming those worked ;)

-            var label = new Clutter.Text.with_text ("Monospace 10", _(@"<b>Player $(worm.id + 1)</b>"));
+            /* Translators: the player's number, e.g. "Player 1" or "Player 2". */
+            var player_id = _("Player %d").printf (worm.id + 1);
+            var label = new Clutter.Text.with_text ("Monospace 10", @"<b>$(player_id)</b>");

Different languages have different rules for plurals. In English, 1 is singular and everything else is plural, but in many languages 0 is plural, and in others there are more categories and strange rules. ngettext gives translators a chance at handling these properly. It fails badly if there are multiple quantities involved, but for a single quantity like we have here, it works well:

-        var points = score > 1 ? "Points" : "Point";
-        var score_label = new Gtk.Label (_(@"<b>$(score) $(points)</b>"));
+        var score_string = ngettext ("%d Point".printf (score), "%d Points".pri
ntf (score), score);
+        var score_label = new Gtk.Label (@"<b>$(score_string)</b>");
Comment 26 Michael Catanzaro 2015-09-20 18:20:45 UTC
(In reply to Michael Catanzaro from comment #25)
but in many languages 0 is plural

I meant "in many languages 0 is singular," in contrast to English.
Comment 27 Michael Catanzaro 2015-09-20 22:16:29 UTC
What if, when a worm dies, it has to stay dead for the rest of the level? That would make the game much less punishing. Currently, if you die, you're likely to die again multiple times in succession.

It would also avoid the question of "what happens when a worm respawns on top of another worm?"
Comment 28 Razvan Chitu 2015-09-21 11:13:26 UTC
Created attachment 311742 [details] [review]
Updated patch for comment 8
Comment 29 Iulian Radu 2015-09-23 10:27:35 UTC
(In reply to Michael Catanzaro from comment #27)
> What if, when a worm dies, it has to stay dead for the rest of the level?
> That would make the game much less punishing. 

You would have to wait quite a bit if you die at the beginning of a level. Doesn't seem to be that fun when playing only with AI.

> Currently, if you die, you're likely to die again
> multiple times in succession.

Well, that probably won't happen if we add a small delay before the worm starts moving.

> It would also avoid the question of "what happens when a worm respawns on
> top of another worm?"

I think that the worms currently passing by the spawn point of the respawning worm should be able to do so without colliding with the respawning worm. We can emphasize that but making the respawning worm transparent and have it stand still until all tiles are clear. What do you think?
Comment 30 Michael Catanzaro 2015-09-23 13:18:51 UTC
(In reply to Iulian Radu from comment #29)
> You would have to wait quite a bit if you die at the beginning of a level.
> Doesn't seem to be that fun when playing only with AI.

Yeah, that is the disadvantage. :/

> Well, that probably won't happen if we add a small delay before the worm
> starts moving.

Right.

> I think that the worms currently passing by the spawn point of the
> respawning worm should be able to do so without colliding with the
> respawning worm. We can emphasize that but making the respawning worm
> transparent and have it stand still until all tiles are clear. What do you
> think?

Sounds fine to me!
Comment 31 Iulian Radu 2015-09-23 21:39:32 UTC
Review of attachment 311742 [details] [review]:

Great!
Comment 32 Razvan Chitu 2015-09-30 16:35:34 UTC
Created attachment 312438 [details] [review]
Patch for respawn delay

(In reply to Iulian Radu from comment #29)
> (In reply to Michael Catanzaro from comment #27)
> > What if, when a worm dies, it has to stay dead for the rest of the level?
> > That would make the game much less punishing. 
> 
> You would have to wait quite a bit if you die at the beginning of a level.
> Doesn't seem to be that fun when playing only with AI.
> 
> > Currently, if you die, you're likely to die again
> > multiple times in succession.
> 
> Well, that probably won't happen if we add a small delay before the worm
> starts moving.
> 
> > It would also avoid the question of "what happens when a worm respawns on
> > top of another worm?"
> 
> I think that the worms currently passing by the spawn point of the
> respawning worm should be able to do so without colliding with the
> respawning worm. We can emphasize that but making the respawning worm
> transparent and have it stand still until all tiles are clear. What do you
> think?

Added a delay after dead worms respawn and made it so keypress events are ignored during the countdown and during respawn.
Comment 33 Gabriel Ivașcu 2015-10-25 19:49:01 UTC
Created attachment 314097 [details] [review]
Move teensy classes, structs and enums at the top

(In reply to Michael Catanzaro from comment #10)
> In nibbles-view.vala, I'd again rather put the teensy classes at the top of
> the file than the bottom. Normally I like to have only one class per file,
> but for these teensy classes, it's pretty hard to justify giving them their
> own files.

Hi, I am a student willing to contribute to GNOME and I'm submitting this as my first patch :)
Comment 34 Michael Catanzaro 2015-10-25 20:42:57 UTC
Thanks Gabriel and Razvan!

Iulian, I think we're at the point where you can review/push these patches, close this bug, file new bugs for the remaining issues, merge everything into master, and release 3.19.1. The biggest issues I see are:

a) Need fix for keypresses not working for a few seconds after the game starts.
b) Need to somehow handle worms respawning on other worms.
c) Still missing the preferences dialog.

But such issues are to be expected at this stage. Worst comes to worst, if we don't have these fixed in time for 3.20, we can create the gnome-3-20 branch off of the gnome-3-18 branch. But I don't think we'll need to do that.
Comment 35 Iulian Radu 2015-10-25 21:42:36 UTC
Review of attachment 312438 [details] [review]:

Good job
Comment 36 Iulian Radu 2015-10-25 21:43:15 UTC
Review of attachment 314097 [details] [review]:

Good job
Comment 37 Gabriel Ivașcu 2015-10-26 20:16:39 UTC
Created attachment 314160 [details] [review]
Remove unused method in NibblesGame class
Comment 38 Gabriel Ivașcu 2015-11-10 08:27:09 UTC
(In reply to Michael Catanzaro from comment #34)

> c) Still missing the preferences dialog.

I am willing to work on that if that's ok with you :)
Comment 39 Michael Catanzaro 2015-11-21 18:57:48 UTC
(In reply to Gabriel Ivascu from comment #38)
> (In reply to Michael Catanzaro from comment #34)
> 
> > c) Still missing the preferences dialog.
> 
> I am willing to work on that if that's ok with you :)

Of course! Thanks for your help Gabriel!