GNOME Bugzilla – Bug 794273
Split five-or-more.c
Last modified: 2018-03-21 01:34:52 UTC
Currently five-or-more.c sourcefile contains most of the code as a spaghetti mix. It should be split out into a couple files: - five-or-more.c should contain the main method, as usual, with a minimal setup of a g_application - five-or-more-app.{c,h} should contain the gapplication-related code and callbacks (startup_cb, activate_cb, shutdown_cb) - game-area.{c,h} should contain the drawing-area-related code (callbacks, animation) - game-preview.{c,h} should contain preview area widget related code. The split can (and probably should be done) step-by-step, separating two components at a time, not doing everything at the same time.
Can I pls take this up?
(In reply to Karuna Grewal from comment #1) > Can I pls take this up? As this is not trivial and will involve several build changes, I would suggest waiting (or even better starting) with the meson port reported in #794026. But if you would prefer starting with this, that's ok too, I'm just trying to reduce the overhead of merging the work done on different branche4s/bugs.
Created attachment 369851 [details] [review] Split the five-or-more.c This patch addresses the splitting up of the code, and it builds fine for me. It'd be nice if you could please review it and let me know if the changes are in compliance with what you expected to be done :)
Review of attachment 369851 [details] [review]: Thanks for working on this. I have commented inline with several suggestions. Other than the inline suggestions, don't add new files without a header for formatting details and license information (given that these files are mostly split from existing files, just copy the header from there, and add your name to the list of authors). I have also added some cleanup requests, maybe missed some, let's see them in the next round. ::: src/five-or-more.c @@ -40,1 @@ #include "games-gridframe.h" Include not needed anymore. games-file-list.h include also not needed, just can't see it in the diff. @@ -40,3 @@ #include "games-gridframe.h" #include "games-scores.h" #include "games-scores-dialog.h" Include not needed anymore. @@ -56,2 @@ #define DEFAULT_BALL_THEME "balls.svg" The #defines are not used at all, so let's just remove them. ::: src/five-or-more.h @@ +4,1 @@ #define STEPSIZE 4 This is not used anymore, it should be removed. @@ +4,2 @@ #define STEPSIZE 4 Given that this file should be empty after all the changes, let's just remove five-or-more.h. @@ +5,3 @@ +GtkWidget **get_scorelabel (); +guint *get_current_score (); These two are kindof strange. They don't really make sense separately. There should be an update_score(int) method in the -app.h which is called from each place where the score has to be updated (games-area.c and games-app.c), the score and the score_label state variables should be kept inside the -app.c. So these methods should not be in any header. ::: src/games-app.c @@ +1,1 @@ +#include <config.h> Let's rename this games-app.{c,h} to five-or-more-app.{c,h}, as games-app is plural, and not really verbose in our context. @@ +104,3 @@ +} + +background git apply complains about trailing whitespace, and is right @@ +116,3 @@ +} + +int git apply complains about trailing whitespace, and is right @@ +212,3 @@ +static void +conf_value_changed_cb (GSettings *settings, gchar *key) +{ git apply complains about trailing whitespace, and is right @@ +220,3 @@ + g_free (color); + } + } else if (strcmp (key, KEY_BALL_THEME) == 0) { git apply complains about trailing whitespace, and is right @@ +221,3 @@ + } + } else if (strcmp (key, KEY_BALL_THEME) == 0) { + gchar *theme_tmp = NULL; git apply complains about trailing whitespace, and is right ::: src/games-app.h @@ +16,3 @@ +char *get_ball_filename (); +background get_backgnd (); +GtkWidget **get_draw_area (); This is not used by anything else but games-area, as it is the internal state variable of that object. Let's just use a GtkWidget internally in that class (set it in draw_area_init, and use it everywhere instead of *(get_draw_area()) ). @@ +24,3 @@ +GSettings **get_settings (); +int get_move_timeout (); +GtkWidget *get_app (); This is only used for creation of a message dialog, let's use NULL instead of this, and remove this from the header. ::: src/games-area.c @@ +1,1 @@ +#include <config.h> Let's rename this to game-area.{c,h}, as it is a drawing area for a single game, not multiple ones. ::: src/games-area.h @@ +28,3 @@ +void set_sizes (gint size); +void reset_game (void); +gint *get_active_status (); This is internal status of the game-area, no need to expose it via a method in header, remove this. @@ +29,3 @@ +void reset_game (void); +gint *get_active_status (); +gint *get_target_status (); This is internal status of the game-area, no need to expose it via a method in header, remove this. @@ +30,3 @@ +gint *get_active_status (); +gint *get_target_status (); +gint *get_inmove_status (); This is internal status of the game-area, no need to expose it via a method in header, remove this. ::: src/games-preview.c @@ +1,1 @@ +#include <config.h> Let's rename this to balls-preview.{c,h}. My initial suggestion was game-preview.{c,h}, but it doesn't make sense, as we are not previewing a game, but the next set of balls. @@ +12,3 @@ + +#include "five-or-more.h" +#include "games-file-list.h" Unnecessary include, remove. @@ +14,3 @@ +#include "games-file-list.h" +#include "games-preimage.h" +#include "games-gridframe.h" Unnecessary include, remove. @@ +15,3 @@ +#include "games-preimage.h" +#include "games-gridframe.h" +#include "games-scores.h" Unnecessary include, remove. @@ +16,3 @@ +#include "games-gridframe.h" +#include "games-scores.h" +#include "games-scores-dialog.h" Unnecessary include, remove.
Created attachment 369865 [details] [review] Split five-or-more.c This incorporates due changes made to address the aforementioned comments. Please check if something more needs to be changed.
Created attachment 369866 [details] [review] Split five-or-more.c Commit message is changed according to the latest changes
Review of attachment 369866 [details] [review]: Ok, looks better now. One more nitpick. ::: src/five-or-more-app.h @@ +48,3 @@ + GVariant *parameter, + gpointer user_data); +void Please match the function header spacing to the rest of the headers, for consistency. That means RETURN TYPE ... (spacing) ... FUNCTION_NAME (ARGUMENT LIST);
Created attachment 369867 [details] [review] Split five-or-more.c Properly aligned the function prototypes in the headers.
Review of attachment 369867 [details] [review]: The new files should be added in po/POTFILES.in, as otherwise we will loose the translations. I've found three more minor problems, after fixing those and the translations, it's ready for merging. ::: src/five-or-more-app.c @@ +643,3 @@ + hbox = GTK_WIDGET (gtk_builder_get_object (builder, "hbox")); + + GtkWidget *draw_area = draw_area_init (); Watch the spacing here. ::: src/game-area.c @@ +1036,3 @@ + +GtkWidget * +draw_area_init (void) Let's call this game_area_init(). ::: src/game-area.h @@ +53,3 @@ +gint *get_active_status (); +gint *get_target_status (); +gint *get_inmove_status (); As said, these three methods (get_active_status, get_target_status and get_inmove_status) are internal representation, and should not appear in the header anymore.
Created attachment 369890 [details] [review] Split five-or-more.c Thanks for the review ! I've made the above changes.
Created attachment 369893 [details] [review] Split five-or-more.c
Review of attachment 369893 [details] [review]: Thanks. I have only found one minor issue with the meson build file due to one of my last requests, a deleted file was not removed from the sources list. I have made that change, and pushed to master. ::: src/meson.build @@ +3,3 @@ five_or_more_c_sources = [ 'five-or-more.c', 'five-or-more.h', This should have been removed after removing the file.
You shouldn't need to list headers at all.