GNOME Bugzilla – Bug 733126
[PATCHes] command-line options
Last modified: 2014-07-14 14:08:23 UTC
Created attachment 280594 [details] [review] Add --version command-line option. Some command-line options should be quite interesting for usual players. Here are four patches: 0001 adds the usual --version for developpers; 0002 adds --paused to begin the game paused, for systems where the game makes time to load or for autolaunching; 0003 adds --size for choosing the size of the board, for autolaunching or command-line addicts; 0004 updates the man page (--size was already documented, with two options that doesn’t exist anymore)
Created attachment 280595 [details] [review] Add --paused command-line option.
Created attachment 280596 [details] [review] Readd --size command-line option.
Created attachment 280597 [details] [review] Update man page.
Review of attachment 280594 [details] [review]: OK ::: src/gnome-tetravex.vala @@ +37,1 @@ private const GLib.ActionEntry[] action_entries = These look like unrelated whitespace changes. (For the better, but they don't belong in this patch.) @@ +231,3 @@ + } + + return -1; I like to add a comment above this return value: /* Activate */
Review of attachment 280595 [details] [review]: OK
Review of attachment 280596 [details] [review]: In this patch you have a lot of unrelated whitespace changes. The ones that don't add line breaks are fine, but as a separate patch rather than shoehorned into this one. I'd prefer not to include the new line breaks, though. ::: src/gnome-tetravex.vala @@ +16,2 @@ private static bool start_paused = false; + private static int game_size = 0; Since you're using handle_local_options, you should be able to avoid this member variable by immediately writing the new value to gsettings. @@ +252,3 @@ + { + stderr.printf (N_("Size could only be from 2 to 6.\n")); + return Posix.EXIT_SUCCESS; EXIT_FAILURE, I think
Review of attachment 280597 [details] [review]: The man page changes are fine. The gnome-tetravex.vala changes should be merged into the appropriate previous patches.
Oh and we need a patch to increase the min GLib version, since local_command_line was introduced in 2.40.
Created attachment 280609 [details] [review] Add --version command-line option. Is that the good way for the GLib thing?
Created attachment 280610 [details] [review] Add --paused
Created attachment 280611 [details] [review] Readd --size command-line option. > Since you're using handle_local_options, you should be able to avoid this > member variable by immediately writing the new value to gsettings. No, I don’t think so. The problem happens when: $ gnome-tetravex -s 2 & # handle command-line, open window, main instance $ gnome-tetravex --version # handle command-line, print version and exit "because there's an EXIT_SUCCESS in the code" gnome-tetravex 3.14.0 $ gnome-tetravex -s 3 # handle command-line, change the variable in gsettings, there’s already a main instance, so bring on top the window of the main instance and silently close the process If the variable is written in gsettings during handle-local-options, then you have weird behavior in the main instance doing this: your game is a 2×2, but the gsettings indicates 3, so if you hit “new game”, you have a 3×3; and before doing that, you cannot hit 3×3 in the app’ menu (see size_changed ()).
Created attachment 280612 [details] [review] Update man page.
Created attachment 280613 [details] [review] Remove white spaces.
Review of attachment 280609 [details] [review]: yup
Review of attachment 280610 [details] [review]: OK
Review of attachment 280611 [details] [review]: OK fair enough. This is good if you split out the renamed sizegroup variable, which I didn't notice before (I assumed it was a whitespace change). Please do rename it, just as a different patch.
Review of attachment 280612 [details] [review]: OK
Review of attachment 280613 [details] [review]: Let me know if you need me to push these.
Created attachment 280615 [details] [review] Readd --size command-line option. Yes please, I cannot push.
Created attachment 280616 [details] [review] Rename "size" variable.
Great Attachment 280609 [details] pushed as b7b3806 - Add --version command-line option. Attachment 280612 [details] pushed as c1ea2fc - Update man page. Attachment 280613 [details] pushed as d63025c - Remove white spaces. Attachment 280615 [details] pushed as 994241d - Readd --size command-line option. Attachment 280616 [details] pushed as ac8180b - Rename "size" variable.