GNOME Bugzilla – Bug 739826
Board size change starts a new game
Last modified: 2014-12-17 17:39:15 UTC
Today, if we change the board size in the preferences a new game is started. I think we should ask for confirmation(to start a new game) when the board size is changed in the preferences. Or we should not start a new game and tell the user that his next game will have the new board size via a dialog box message / in the header bar( below Five or More ).
(In reply to comment #0) > Today, if we change the board size in the preferences a new game is started. > > I think we should ask for confirmation(to start a new game) when the board size > is changed in the preferences. Yes, absolutely. > Or > > we should not start a new game and tell the user that his next game will have > the new board size via a dialog box message / in the header bar( below Five or > More ). This would be better than the status quo, but I'd prefer the first approach instead.
Does this text look ok? ----------------------------------- | Would you like to restart the game? | | | | Restart | Cancel | -----------------------------------
How about: "If you change your board size, the game will be restarted." Cancel Restart (In GNOME, the affirmative button is always on the right, so the user sees Cancel first.) Robert, does that sound good to you, or do you have another preference here?
Created attachment 291303 [details] [review] Ask before restarting the game on board size change Patch adding a restart_game_dialog. 1. Ask before restarting the game when the player changes the board size. 2. Doesn't display the dialog when the user presses the radio button which is the current game size. --- Michael, Robert: How about keeping it really simple without any text?
Review of attachment 291303 [details] [review]: ::: src/five-or-more.c @@ +1300,3 @@ + if (pref_dialog_done && !restart_game_dialog && game_size != data) { + GtkDialogFlags flags = GTK_DIALOG_DESTROY_WITH_PARENT; + restart_game_dialog = gtk_dialog_new_with_buttons ("Restart Game", Nah, it should be a sentence or it will be confusing: something along the lines of "do you want to restart the game?" @@ +1319,3 @@ + case SMALL: + size_radio = GTK_WIDGET (gtk_builder_get_object (builder_preferences, "radiobutton_small")); + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (size_radio), TRUE); Since these did not need to be set before, why is it that they must be now? @@ -1410,3 @@ G_CALLBACK (set_fast_moves_callback), NULL); - g_object_unref (G_OBJECT (builder_preferences)); Surely you're leaking this now?
(In reply to comment #5) > Review of attachment 291303 [details] [review]: > > ::: src/five-or-more.c > @@ +1300,3 @@ > + if (pref_dialog_done && !restart_game_dialog && game_size != data) { > + GtkDialogFlags flags = GTK_DIALOG_DESTROY_WITH_PARENT; > + restart_game_dialog = gtk_dialog_new_with_buttons ("Restart Game", > > Nah, it should be a sentence or it will be confusing: something along the lines > of "do you want to restart the game?" Ok. ^(^ > > @@ +1319,3 @@ > + case SMALL: > + size_radio = GTK_WIDGET (gtk_builder_get_object (builder_preferences, > "radiobutton_small")); > + gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (size_radio), TRUE); > > Since these did not need to be set before, why is it that they must be now? I did this to revert to the original game size radio button incase the player presses cancel. Same is the reason for not unref-ing below. > > @@ -1410,3 @@ > G_CALLBACK (set_fast_moves_callback), NULL); > > - g_object_unref (G_OBJECT (builder_preferences)); > > Surely you're leaking this now? Wont this be cleaned up once the preferences window is closed? Not doing this was causing me seg faults when I was trying to revert to the original game size radio button showing up incase the user presses cancel( preferences -> size change -> Cancel restarting the game). May be I should think harder for increasing/decreasing reference counts properly incase g_object_unref is required.
(In reply to comment #6) > Wont this be cleaned up once the preferences window is closed? No, the GtkBuilder is not a GtkWidget, so it doesn't become part of the normal widget hierarchy. You have to clean it up manually. It's a wrapper around the XML UI file that's used to create widgets, and it's mostly the contents of the file that are being leaked here (as opposed to the actual preferences dialog). > Not doing this was causing me seg faults when I was trying to revert to the > original game size radio button showing up incase the user presses cancel( > preferences -> size change -> Cancel restarting the game). > > May be I should think harder for increasing/decreasing reference counts > properly incase g_object_unref is required. Well, you have one reference to the builder, which is all you need, so you can unref it exactly once and be fine (and it's already unreffed in the correct place). But if you unref it before size_callback(), it will be destroyed, and then when you try to use it in size_callback() that's not going to work. One solution would be to store a file-scope pointer to each radio button, so you don't have to try to get them from the builder again. (This is a typical strategy: it's not usual to get the same widget from a builder multiple times.) Another improvement would be to change the setting only after the user clicks Restart, instead of changing it and then immediately changing it back.
Created attachment 291744 [details] [review] Ask before restarting the game on board size change 1. Un-reffing correctly and using file scope pointers instead. -Done 2. change the setting only after the user clicks Restart, instead of changing it and then immediately changing it back. -This is working in a callback, Its the radio button(Preferences) getting selected that prompts the "Restart Game" dialog. Is there something I'm missing?
Review of attachment 291744 [details] [review]: Now this patch works great. Minor comments only: Can you change this so that it uses GtkMessageDialog instead of directly using GtkDialog? That will simplify the code and result in a better-looking dialog. ::: src/five-or-more.c @@ +99,3 @@ static GtkWidget *draw_area; +static GtkWidget *app, *headerbar, *pref_dialog, *gridframe, *restart_game_dialog; +static GtkWidget *size_radioS, *size_radioM, *size_radioL; Normally we only use lowercase letters in variable names. @@ +1299,3 @@ + game_size = g_settings_get_int (settings, KEY_SIZE); + + if (pref_dialog_done && !restart_game_dialog && game_size != data) { Since you always destroy restart_game_dialog, there's no need to check if it's NULL, correct?
Created attachment 291829 [details] [review] Ask before restarting the game on board size change +Using GtkMessageDialog instead of GtkDialog +Using lowercase letters in variable names restart_game_dialog is required to avoid displaying the restart dialog twice when the player presses cancel.
Review of attachment 291829 [details] [review]: OK great. Last comments from me; I'll leave this a couple days in case Robert wants to review it; otherwise, it's fine after changing: ::: src/five-or-more.c @@ +1,1 @@ + Accidental blank line here... @@ +1307,3 @@ + GTK_BUTTONS_OK_CANCEL, + "Are you sure you want to restart the game?"); + gtk_window_set_position (GTK_WINDOW (restart_game_dialog), GTK_WIN_POS_MOUSE); You [don't want, don't need] to call gtk_window_set_position()
Created attachment 291840 [details] [review] Ask before restarting the game on board size change Michael, Thanks for the quick review! +Got rid of gtk_window_set_position()
Shouldn't the new dialog be translatable?
Of course, good catch. (By what superpowers did you discover this? Do you have a list new untranslatable strings, in addition to translatable strings?)
The following fixes have been pushed: d9e14e4 Use an imperative verb for the new dialog's action 4641846 Mark new string for translation
Created attachment 292914 [details] [review] Use an imperative verb for the new dialog's action https://developer.gnome.org/hig/stable/dialogs.html
Created attachment 292915 [details] [review] Mark new string for translation
(In reply to comment #14) > Of course, good catch. (By what superpowers did you discover this? Do you have > a list new untranslatable strings, in addition to translatable strings?) I like to browse through recent changes in GNOME git in my free time. Thanks for a quick fix!