GNOME Bugzilla – Bug 664943
Port to Vala
Last modified: 2012-03-10 13:41:41 UTC
Jason, I believe you have someone working on this?
See bug 664944 about adding GSettings support. I'm assuming that will be done as part of the port.
I think the person working on this was part of the GNOME Outreach Program: https://live.gnome.org/GnomeWomen/OutreachProgram2011
I'm the intern of GNOME Outreach Program to work on this bug, plus lightsoff
Lightsoff has already been completed, see bug 664410
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.
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 Makefile.am, 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
Created attachment 208393 [details] [review] Vala swell-foop (I've converted the tarball to a patch so it's easier to review)
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 http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller). 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). e.g. 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: try { 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); }
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!
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?
ok from me to land this. We need to be careful not to change strings or UI as there is freeze on those.
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.
Created attachment 209230 [details] [review] Patch fixing some final details Updated to fix the score label so it's not a string break.
Freeze break requested: http://mail.gnome.org/archives/release-team/2012-March/msg00045.html
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 themes.
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.
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.
Fantastic work, Sophia. And thank you for your hard work, too, Robert.
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_TYPE_STRING) || g_variant_is_of_type (value, G_VARIANT_TYPE_OBJECT_PATH) || g_variant_is_of_type (value, G_VARIANT_TYPE_SIGNATURE)' failed'`
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.
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.
(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/POTFILES.in so xgettext does not break, namely by adding swell-foop/data/preferences.ui and removing swell-foop/data/settings.ui swell-foop/src/About.js swell-foop/src/Board.js swell-foop/src/Light.js swell-foop/src/Score.js swell-foop/src/Settings.js swell-foop/src/ThemeLoader.js You can try yourself by running e.g. "intltool-update --pot" in the /po subdirectory.
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...
I found the bug (something weird to do with how clutter was initialized). UI freeze got approved: https://mail.gnome.org/archives/release-team/2012-March/msg00058.html Pushed! Congratulations Sophia!