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 664943 - Port to Vala
Port to Vala
Product: gnome-games-superseded
Classification: Deprecated
Component: swell-foop
Other Linux
: Normal normal
: ---
Assigned To: Jason Clinton
GNOME Games maintainers
Depends on:
Blocks: 671618
Reported: 2011-11-27 22:30 UTC by Robert Ancell
Modified: 2012-03-10 13:41 UTC
See Also:
GNOME target: ---
GNOME version: ---

Vala swell-foop (113.39 KB, patch)
2012-02-22 08:24 UTC, Sophia Yu
none Details | Review
Vala swell-foop (52.03 KB, patch)
2012-02-25 09:28 UTC, Robert Ancell
needs-work Details | Review
Second attempt to port swell-foop from JS to Vala (92.53 KB, patch)
2012-03-05 08:48 UTC, Sophia Yu
needs-work Details | Review
Patch fixing some final details (32.05 KB, patch)
2012-03-08 01:16 UTC, Robert Ancell
none Details | Review
Patch fixing some final details (32.05 KB, patch)
2012-03-08 01:25 UTC, Robert Ancell
none Details | Review
The third attempt to port swell-foop from JS to Vala (96.55 KB, patch)
2012-03-08 07:03 UTC, Sophia Yu
none Details | Review
Vala swell-foop (103.30 KB, patch)
2012-03-09 04:06 UTC, Robert Ancell
none Details | Review

Description Robert Ancell 2011-11-27 22:30:16 UTC
Port to Vala
Comment 1 Robert Ancell 2011-11-27 22:30:51 UTC
Jason, I believe you have someone working on this?
Comment 2 Robert Ancell 2011-11-27 23:29:53 UTC
See bug 664944 about adding GSettings support.  I'm assuming that will be done as part of the port.
Comment 3 Robert Ancell 2011-12-05 23:32:24 UTC
I think the person working on this was part of the GNOME Outreach Program:
Comment 4 Sophia Yu 2011-12-18 08:22:25 UTC
I'm the intern of GNOME Outreach Program to work on this bug, plus lightsoff
Comment 5 Robert Ancell 2011-12-19 02:50:20 UTC
Lightsoff has already been completed, see bug 664410
Comment 6 Sophia Yu 2011-12-19 04:46:01 UTC
Right, I just noticed that lightsoff have been done today. So I think I would like to help as mush as possible the tasks listed in the modernization page.
Comment 7 Sophia Yu 2012-02-22 08:24:05 UTC
Created attachment 208182 [details] [review]
Vala swell-foop

The attached tar.gz file is the compression of the subdirectory of swell-foop in the code tree. It contains the Vala code, modified, modified data files. Note that the original Javascript files were not removed. 

I'm not sure if it is better to upload a tar ball instead of a patch. I guess it is more flexible to use a tar ball for many file replacement changes. 

The code is still in rough status. The preference setting and the score animation are both working. 

I upload this tar ball with the purposes of receiving code reviews and helps from other contributors. Hopefully it can be included in the coming release of GNOME 3.4
Comment 8 Robert Ancell 2012-02-25 09:28:13 UTC
Created attachment 208393 [details] [review]
Vala swell-foop

(I've converted the tarball to a patch so it's easier to review)
Comment 9 Robert Ancell 2012-02-25 09:50:02 UTC
Review of attachment 208393 [details] [review]:

Hi Sophia.  Good start!  It runs well and has the same behaviour as the existing game which is good.  Some functionality missing as you say.

Some general comments:
- Make sure to be consistent with coding style.  We don't have a formal coding style for GNOME Games but in general when contributing to a project make sure to match the existing style.
- Don't have dead code in patches - it makes them harder to review.  In this case I think you have some here because I pushed you to put the patch here :)
- Make sure to comment your code!
- Keep your UI code separate from the rest of the code (see  It seems more work to start with but it makes the code much easier to read and easier to maintain into the future.

::: swell-foop/src/board-view.vala
@@ +11,3 @@
+    public bool visited = false;
+    public Light ( Clutter.Texture on_texture )

Whitespace consistency...

@@ +116,3 @@
+public class BoardView : Clutter.Group

Rename this to GameView - this game doesn't represent a physical board game so Game is probably more appropriate.  This class should be split into a "Game" and a "GameView" class - the game logic should be in the Game class and the Clutter rendering in the GameView class.

@@ +118,3 @@
+public class BoardView : Clutter.Group
+    private Settings settings;

Move the settings object into the SwellFoop class and have properties on the GameView class - this makes the class more flexible, e.g. if we want to have more than one view and each view to have different settings.  (For example how the themes are shown Quadrapassel).


private string? _theme = null;
public string theme
    get { return _theme; }
    set { _theme = value; redraw_board (); }

Then from the SwellFoop class you can do the following:

var view = new BoardView ();
view.theme = settings.get_string ("theme");

@@ +136,3 @@
+	/**/
+	/*if(theme == null)*/
+	/*	theme = themes[default_theme];*/

I'm sure these are here to remind you of things to be implemented, make sure any final patch should not have any dead code.

@@ +178,3 @@
+        add_actor (yellow_texture);
+        red_texture.hide ();
+        add_actor (red_texture);

Add a comment before large blocks like this, e.g. /* Create the textures required to render */

@@ +234,3 @@
+        if (li.visited || li.closed) {
+            return cl;
+        }

Don't need curly braces when only have one line in an if statement - be consistent with the rest of the code (and consistent with the other games).

::: swell-foop/src/swell-foop.vala
@@ +13,3 @@
+        settings = new Settings ("org.gnome.swell-foop");
+        ui = new Gtk.Builder();

Make sure there is a space before parenthesis

@@ +100,3 @@
+    [CCode (cname = "G_MODULE_EXPORT select_theme", instance_pos = -1)]
+	public void select_theme (Gtk.Widget widget)

Note here there is mixed tab characters and spaces.  You should only use 4 spaces for indentation.

@@ +174,3 @@
+    public void help_cb (Gtk.Widget widget)
+    {
+        /*GnomeGamesSupport.help_display (window, "swell-foop", null);*/

This is an easy fix:
            Gtk.show_uri (window.get_screen (), "help:swell-foop", Gtk.get_current_event_time ());
        catch (Error e)
            warning ("Failed to show help: %s", e.message);
Comment 10 Sophia Yu 2012-03-05 08:48:28 UTC
Created attachment 208979 [details] [review]
Second attempt to port swell-foop from JS to Vala

Please check this out. This newly baked patch contains many changes from the first one. Let me know how do you feel like it so that I can continue improve. Thanks a lot!
Comment 11 Robert Ancell 2012-03-07 22:59:12 UTC
Review of attachment 208979 [details] [review]:

I'm really impressed.  This is a major improvement over the last patch and basically ready for release once we get the theming, high score and window resizing code working.  The code structure and commenting was very easy to understand, and this code is high quality.  Well done Sophia!

::: swell-foop/src/game-view.vala
@@ +34,3 @@
+    private Clutter.Actor on_actor;
+    /* Properties */

Note a useful comment (the syntax makes it obvious these are properties).  Have a comment per property describing what each one does. e.g.
/* The size of this tile in pixels */
public int tile_size { get; set; default = 50; }

@@ +40,3 @@
+    public int grid_y { get; set; default = 0; }
+    /* Constructor */

Not a useful comment (it's obvious it's the constructor)

@@ +82,3 @@
+    public void hide_light_cb ()
+    {
+        this.hide();

whitespace after method name and args

@@ +226,3 @@
+    {
+        get { return _theme_name; }     
+        set { _theme_name = value; }

As mentioned in swell-foop.vala, if the theme name changes on set then update the each LightActor to use the new theme.

::: swell-foop/src/swell-foop.vala
@@ +118,3 @@
+        /* show the current score */
+        // FIXME Is it a good place to initialize the score label? (sophiay)

I'd set it to "" then call update_score_cb so the code is in one place

@@ +174,3 @@
+        // TODO high score is not supported (sophiay)
+        high_scores = new GnomeGamesSupport.Scores ("swell_foop",  /* FIXME what is a good name here? swellfoop? swell-foop? */

Use swell-foop to be consistent with the other names and also the same as what the Javascript version used.

@@ +247,3 @@
+        // TODO new_game() seems not the best way to response. Do we allow a non-start-over theme
+        // changing? (sophiay)

We should support changing the theme without restarting - do view.theme = new_theme and let the view update itself.

@@ +353,3 @@
+        view.is_zealous = settings.get_boolean ("zealous");
+        // FIXME When change from bigger size to smaller one, the window size doesn't change.

I'm not sure the correct way to do this, I think you can do a window.queue_resize?
Comment 12 Thomas Andersen 2012-03-07 23:10:00 UTC
ok from me to land this. We need to be careful not to change strings or UI as there is freeze on those.
Comment 13 Robert Ancell 2012-03-08 01:16:53 UTC
Created attachment 209228 [details] [review]
Patch fixing some final details

This patch fixes a number of things broken/missing in Sophia's last patch (I would have left you to do these but I'd love to get it into 3.4 asap).

There's one thing blocking this being released that I can't work out.  For me I can't click on the blocks and the enter leave event only works when entering and leaving the window edge.  Sophia - can you see if you get the same/investigate what's going wrong?

Due to the toolbar and GMenu changes in this patch we will need a freeze request, hopefully it wont be controversial.
Comment 14 Robert Ancell 2012-03-08 01:25:18 UTC
Created attachment 209230 [details] [review]
Patch fixing some final details

Updated to fix the score label so it's not a string break.
Comment 15 Robert Ancell 2012-03-08 01:30:57 UTC
Freeze break requested:
Comment 16 Sophia Yu 2012-03-08 02:27:24 UTC
For the enter/leave event, the game works all right at my Ubuntu 11.10. 

But when a game is complete, in the new game, the old score actor is still on the
screen. Do you see it? In my old code, I call remove_all() and re-create
Comment 17 Robert Ancell 2012-03-08 02:33:31 UTC
I don't see the problem because I can't complete a game.  remove_all () is actually deprecated in newer versions of Clutter so I removed it.  So we need to change it to calling destroy on the score actor when starting a new game.
Comment 18 Robert Ancell 2012-03-08 02:34:33 UTC
I'm using clutter 1.8.4 on Precise, but the Javascript version works with that and I can't work out what might have changed.
Comment 19 Jason Clinton 2012-03-08 03:44:07 UTC
Fantastic work, Sophia. And thank you for your hard work, too, Robert.
Comment 20 Sophia Yu 2012-03-08 04:08:55 UTC
Why I saw this error message thrown from swell-foop.vala:185 setting.get_string
("size")? I located the line in gdb. dconf-editor shows the size is in string
type. I'm using jhbuild version gtk/glib

Glib-CRITICAL **: g_variant_get_string: assertion `g_variant_is_of_type (value,
|| g_variant_is_of_type (value, G_VARIANT_TYPE_OBJECT_PATH) ||
g_variant_is_of_type (value,
Comment 21 Robert Ancell 2012-03-08 04:20:01 UTC
Make sure you're running with the latest schema - it was an integer before, but it's been changed to an enumeration which is more appropriate.
Comment 22 Sophia Yu 2012-03-08 07:03:48 UTC
Created attachment 209233 [details] [review]
The third attempt to port swell-foop from JS to Vala

This patch combines mine and Robert's efforts to port swell-foop from Javascript to Vala. The changes from Robert's patch is I have made the score actor as a private member of game-view so that we won't need to create score actors everytime a score animation is played(the old way). When a game starts it is easier to clear the old score.

NB: I'm kinda still not feel confident enough to guarantee this patch is a all-in-one because I'm not feel home when squash multiple commits. Even though I have tested it in a new branch, please test it again on your computer.
Comment 23 André Klapper 2012-03-08 11:37:59 UTC
(In reply to comment #22)
> Created an attachment (id=209233) [details] [review]
> The third attempt to port swell-foop from JS to Vala

Applying with "git am" I get
"warning: squelched 48 whitespace errors
warning: 53 lines add whitespace errors."

Also the patch needs to update /po/ so xgettext does not break, namely by adding
and removing
You can try yourself by running e.g. "intltool-update --pot" in the /po subdirectory.
Comment 24 Robert Ancell 2012-03-09 04:06:29 UTC
Created attachment 209298 [details] [review]
Vala swell-foop

Patch updated with the last change I had.  Also removed some dead code, removed references to "light" (copied from lightsoff - they're no lights in swell foop), fixed gettext stuff and a invalid memory access in the code.  Still doesn't handle click events for me and can't work out why...
Comment 25 Robert Ancell 2012-03-10 13:41:41 UTC
I found the bug (something weird to do with how clutter was initialized).

UI freeze got approved:

Pushed!  Congratulations Sophia!