GNOME Bugzilla – Bug 559298
Make able to manually enter puzzles
Last modified: 2015-04-21 17:26:45 UTC
The start page should an option for an empty sudoku board. This could be used for solving puzzles found elsewhere. Machine generated puzzles are rarely interesting to solve or difficult enough.
The gnome-sudoku feature of being able to enter a custom game existed until recently (as distributed under Ubuntu Gutsy Gibbon for example). I like to be able to type in a game from the newspaper sometimes. One can uninstall the current version, and revert to an older one. However I can't imagine why this useful feature got dropped in the first place. This should be considered a valuable extension.
*** Bug 559751 has been marked as a duplicate of this bug. ***
Created attachment 132329 [details] [review] Old code for typing in sudokus by hand This is the code that used to be the feature for typing sudokus by hand. I'm posting it here in the case someone wants to revive this feature. The code should be reviewed/tested and functionality to test that the hand type sudoku is valid should be added. Considerations about improvements to the GUI/interaction for this would also be welcome. This is a very wanted feature and may not require much work. We should push to get this done during 2.27
*** Bug 597221 has been marked as a duplicate of this bug. ***
So, excuse my ignorance, why was removed from gnome-sudoku the support to add manual sudokus?
I believe it was due to the code being unstable and causing a lot of problems. No feature is better than a broken feature.
I would like some details about how this should look (and maybe some details about the implementation, if you have preferences).
Here's an idea - We can add an option in the New Game screen with a label something like "Create your own puzzle" which will take us to the Game Editor screen. This screen will be the same current Game screen with a couple of changes to allow editing. We'll give the user an empty board to start with, no fixed cells, no filled cells. Clicking on a square will spawn the popover with the usual number-pad and "Set fixed/Unset fixed", "Clear" buttons. We should allow to set a value in a cell AND let the cell be unfixed for the scenario when the user wants to continue a game he was already playing somewhere else. In the right sidebar, "Clear Board" and "New Puzzle" buttons will be retained, and a "Done" button will replace "Pause" which will turn the editing off, and start the game, and everything will proceed as normal. Timer will start and the "Done" will change to "Pause".
(In reply to Parin Porecha from comment #8) > We'll give the user an empty board to start with, no fixed cells, no filled > cells. Clicking on a square will spawn the popover with the usual number-pad > and "Set fixed/Unset fixed", "Clear" buttons. We should allow to set a value > in a cell AND let the cell be unfixed for the scenario when the user wants > to continue a game he was already playing somewhere else. I don't understand this bit: if the user wants to continue a game he was already playing somewhere else, he could create the fixed spots at first, then hit Done and start filling in the non-fixed spots? Otherwise, sounds good.
I was assuming that the user would be copying the puzzle from a paper or somewhere cell by cell mechanically, and then when he's done he'll want to start playing at the point he left off. But if we do it your way, we won't need the 'Set fixed/Unset fixed' button. The cell which gets filled will automatically be made fixed. The 'Clear' button will clear the value and also clear the fixed property. This will be much simpler to implement and use plus less chances of mistakes. Good point.
(In reply to Parin Porecha from comment #10) > I was assuming that the user would be copying the puzzle from a paper or > somewhere cell by cell mechanically, and then when he's done he'll want to > start playing at the point he left off. Well I'm assuming that too, I just don't see any difference between letting the user fill in those squares before or after clicking the Done button. :)
Created attachment 300858 [details] [review] Patch to add the ability to enter puzzles manually Hello, I've attached the patch which adds the ability to manually enter puzzles. The new game screen now has an option at the bottom 'Create your own puzzle' which takes the user to the puzzle creation screen which is essentially an empty board with a couple of changes to allow editing - - The editor allows for creating fixed cells only (as we decided above). - Earmarks and Timer are disabled. Right-clicking a cell will have no effect, the earmark-picker will not spawn and Ctrl+number will not add earmarks to a cell but will set the cell value to that number instead. - Undo/Redo and Clear Board work just like while playing a puzzle. - When the editing is complete, the user can click on the 'Start playing' button upon which the game will start just like an inbuilt puzzle. Timer will start, earmarks will be enabled, etc. etc. - If the user closes the application in the middle of entering a puzzle, the state will be saved and on restarting he can continue editing where he left off. One of the advantages of using the same code for puzzle creation as well is the built-in error checking. By turning on the "Warnings" option, user can know if the puzzle he is entering is correct on a basic level. Of course puzzle solving can be applied at every step to ensure the puzzle is indeed solvable, but that would be at a considerable expense of computation power. Screenshots - New game screen - http://i.imgur.com/cBkVPsP.png Puzzle creation screen - http://i.imgur.com/fXpqgRC.png
Review of attachment 300858 [details] [review]: Cool! I don't really like the separator above the new button. It'd be nice to have the space there without the separator. (In reply to Parin Porecha from comment #12) > By turning on the "Warnings" option, user can > know if the puzzle he is entering is correct on a basic level. I think we should display warnings during puzzle creation regardless of the value of the setting. The only reason it's a setting is to allow users to play without hints; during puzzle creation, you definitely want to know if your puzzle is impossible. > Of course > puzzle solving can be applied at every step to ensure the puzzle is indeed > solvable, but that would be at a considerable expense of computation power. I think we should be sure the final puzzle is solvable, so the user doesn't wind up playing an impossible game. You can use QQwing for this; SudokuBoard::solve should do the trick. I don't doubt there could be performance issues if you attempt to solve the puzzle whenever it changes, but hopefully that will be fast enough to run the check when pressing the Start Playing button. (A spinner will be needed if it takes more than a second or two.) If the puzzle has no possible solution, just pop up a message dialog I suppose. ::: data/gnome-sudoku.ui @@ +234,3 @@ + <property name="visible">False</property> + <property name="use_underline">True</property> + <property name="label" translatable="yes">_Start playing</property> Use a capital P ::: lib/sudoku-board.vala @@ +609,3 @@ return _("Very Hard Difficulty"); + case CUSTOM: + return _("Custom Difficulty"); Um... but the user didn't create a custom difficulty, he created a custom puzzle. So I would say "Custom Puzzle." :D ::: src/gnome-sudoku.vala @@ +212,3 @@ + if (savegame != null) { + if (savegame.board.difficulty_category == DifficultyCategory.CUSTOM) + current_game_mode = savegame.board.filled == savegame.board.fixed ? GameMode.CREATE : GameMode.PLAY; Heh, that's neat. @@ +499,3 @@ + headerbar.title = game.board.difficulty_category.to_string (); + else + headerbar.title = _("Create your own puzzle"); I would use a shorter title: "Create Puzzle"
Finishing a custom grid opens two “play again/quit” popups instead of one. By the way, hitting <Esc> closes these popups and lets the game paused, with “New Grid” insensitive, and the “Clear Grid” sensitive (even if we don’t see the grid, as the game is paused).
Completely (correctly) filling the grid in the create-grid mode makes the “You win” popup appear.
(In reply to Arnaud Bonatti from comment #14) > Finishing a custom grid opens two “play again/quit” popups instead of one. Thanks for noticing this! I'll fix it. > By the way, hitting <Esc> closes these popups and lets the game paused, with > “New Grid” insensitive, and the “Clear Grid” sensitive (even if we don’t see > the grid, as the game is paused). That's a bug in the game completion dialog. It also happens when you complete a inbuilt puzzle. Can you please file a bug for this?
(In reply to Arnaud Bonatti from comment #15) > Completely (correctly) filling the grid in the create-grid mode makes the > “You win” popup appear. Because of the above fix, the popup won't appear anymore. Instead, if this scenario happens, the 'Start Playing' button will be disabled.
Created attachment 300913 [details] [review] Updated patch to add ability to enter puzzles manually (In reply to Michael Catanzaro from comment #13) > I don't really like the separator above the new button. It'd be nice to have > the space there without the separator. Removed the separator and added a 30px margin. > (In reply to Parin Porecha from comment #12) > > By turning on the "Warnings" option, user can > > know if the puzzle he is entering is correct on a basic level. > > I think we should display warnings during puzzle creation regardless of the > value of the setting. The only reason it's a setting is to allow users to > play without hints; during puzzle creation, you definitely want to know if > your puzzle is impossible. I wrote the code for this, but during testing I ran into couple of scenarios where there was no way to avoid inconsistency in the "Warnings" option. Turns out saving the previous state of the warnings action on starting the puzzle creator and then setting the action to that saved state on starting a game causes inconsistencies on loading a saved game. If we ignore the setting and display warnings regardless its value, that's also an irregularity - Option says OFF but warnings are still being shown. Also, what if the user turns the option OFF while creating a puzzle? The next time he starts creating one, he'll expect it to be the way he left, but no it will be ON. > > Of course > > puzzle solving can be applied at every step to ensure the puzzle is indeed > > solvable, but that would be at a considerable expense of computation power. > > I think we should be sure the final puzzle is solvable, so the user doesn't > wind up playing an impossible game. You can use QQwing for this; > SudokuBoard::solve should do the trick. I don't doubt there could be > performance issues if you attempt to solve the puzzle whenever it changes, > but hopefully that will be fast enough to run the check when pressing the > Start Playing button. (A spinner will be needed if it takes more than a > second or two.) If the puzzle has no possible solution, just pop up a > message dialog I suppose. Wow, this would be a very nice addition. But to keep this patch focused on only the basic puzzle creation feature, I'll post a separate patch for this. I've included the rest of your and Arnaud's suggestions.
Review of attachment 300913 [details] [review]: > If we ignore the setting and display warnings regardless its value, that's > also an irregularity - Option says OFF but warnings are still being shown. I think that's fine. The point of being able to turn off warnings is to be able to play the game without getting extra help from the computer. But when creating a puzzle, it would just be silly not to show warnings. Nobody wants to enter an entire puzzle, only to find out at the end that something unspecified is wrong with the puzzle. Best show the warnings as soon as possible, always. > Also, what if the user turns the option OFF while creating a puzzle? The > next time he starts creating one, he'll expect it to be the way he left, but > no it will be ON. I think that puzzle creation should have absolutely no effect on the setting. If the user turns the option OFF while creating a puzzle, warnings should continue to be displayed during puzzle creation, but the option be off and stay off when the game begins. > (In reply to Michael Catanzaro from comment #13) > Wow, this would be a very nice addition. But to keep this patch focused on > only the basic puzzle creation feature, I'll post a separate patch for this. OK, great. Though scratch my previous comment; it would be better to use SudokuBoard::count_solutions rather than SudokuBoard::solve, so you can make sure there is exactly one solution to the puzzle. (This is what's done when generating.)
Created attachment 300938 [details] [review] Patch to add ability to create puzzles manually (In reply to Michael Catanzaro from comment #19) > Review of attachment 300913 [details] [review] [review]: > > > If we ignore the setting and display warnings regardless its value, that's > > also an irregularity - Option says OFF but warnings are still being shown. > > I think that's fine. The point of being able to turn off warnings is to be > able to play the game without getting extra help from the computer. But when > creating a puzzle, it would just be silly not to show warnings. Nobody wants > to enter an entire puzzle, only to find out at the end that something > unspecified is wrong with the puzzle. Best show the warnings as soon as > possible, always. Fair enough. I've updated the patch to enable warnings in creation mode regardless of the setting value. On starting a game, the saved value will be restored.
Review of attachment 300938 [details] [review]: Looks good to me. I'd mark this as accepted except I want the check that the board is valid. :) ::: data/gnome-sudoku.ui @@ +220,3 @@ <property name="spacing">6</property> <child> + <object class="GtkButton" id="play_custom_game_button"> I think the "primary" action is supposed to be the one on the bottom, if I remember Allan's advice properly, so try moving this to the bottom.
(In reply to Michael Catanzaro from comment #22) > ::: data/gnome-sudoku.ui > @@ +220,3 @@ > <property > name="spacing">6</property> > <child> > + <object > class="GtkButton" id="play_custom_game_button"> > > I think the "primary" action is supposed to be the one on the bottom, if I > remember Allan's advice properly, so try moving this to the bottom. Okay. One more thing - I'm reading this - https://developer.gnome.org/hig/stable/visual-layout.html.en , and I think that the 30px margin I've set b/w the predetermined difficulty buttons and the 'Create your own puzzle' button in the new game screen [1] might be wrong. What do you think? [1] http://i.imgur.com/TW9wqJ5.png
I think the start screen is a bit different from a dialog full of content, so I don't think those guidelines apply here. It would be great to have a designer look over the screen though (probably Allan, since it's his design we're breaking :); maybe they'll want headings or something.
(In reply to Michael Catanzaro from comment #22) > Review of attachment 300938 [details] [review] [review]: > > Looks good to me. I'd mark this as accepted except I want the check that the > board is valid. :) Coming in the next patch. > ::: data/gnome-sudoku.ui > @@ +220,3 @@ > <property > name="spacing">6</property> > <child> > + <object > class="GtkButton" id="play_custom_game_button"> > > I think the "primary" action is supposed to be the one on the bottom, if I > remember Allan's advice properly, so try moving this to the bottom. Done. Pushed it to a new branch 'manual-puzzle-creator'. (In reply to Michael Catanzaro from comment #24) > I think the start screen is a bit different from a dialog full of content, > so I don't think those guidelines apply here. It would be great to have a > designer look over the screen though (probably Allan, since it's his design > we're breaking :); maybe they'll want headings or something. I've contacted Allan about this. Lets merge the branch since the basic feature is complete. I'll make the changes on getting his reply.
Review of attachment 300938 [details] [review]: Pushed [1]. [1] https://git.gnome.org/browse/gnome-sudoku/commit/?id=092d2f625fc7edeebb3f67016e555b2269430071
Created attachment 301889 [details] [review] Add validity and uniqueness check to manually created puzzles Due to this check, a game will start only if the puzzle entered has a unique possible solution. Error dialog shown on entering a invalid puzzle - http://i.imgur.com/fltnY9m.png And one with multiple solutions - http://i.imgur.com/0upZ0a6.png
Review of attachment 301889 [details] [review]: Looks good, my comments are all nits, as usual. Thanks! :) ::: lib/qqwing-wrapper.cpp @@ +69,3 @@ + * Returns 0 if the puzzle is not valid. + */ +int qqwing_count_solutions_limited(int *puzzle) In C++ we normally put the * next to the data type, rather than the variable name like we normally do in C. At least, that's the style I adopted in this file. (See also the function declaration below.) ::: lib/qqwing.vapi @@ +24,3 @@ [CCode (array_length=false)] int[] generate_puzzle (int difficulty); + int count_solutions_limited (int *puzzle); Hm... I'd rather avoid using pointers in Vala. Can't we use an array here, like we do for print_stats()? int count_solutions_limited ([CCode (array_length = false)] int[] puzzle); ::: lib/sudoku-board.vala @@ +395,3 @@ + public int count_solutions_limited () + { + int[] puzzle = new int[rows * cols]; I guess it's not possible to use SudokuBoard.cells here, correct? @@ +400,3 @@ + puzzle[(row * cols) + col] = cells[row, col]; + + return QQwing.count_solutions_limited (puzzle); OK, this looks good. I didn't realize you could pass an array to a vala function expecting a pointer. Still, it would be good to change the vapi to take an array, as well. ::: src/gnome-sudoku.vala @@ +304,3 @@ + var error_str = ""; + if (solutions == 0) + error_str = _("The puzzle you have entered is not a valid Sudoku.\nPlease enter a valid puzzle."); OK, but a couple of i18n thoughts: * Please add a translator comment on the line above. (I normally also add braces to the if statement when I do this, even though not necessary, since it gets hard to read without braces when the body is multiple lines.) * Let's avoid using \n in the translator string. Do something like String.printf("%s\n%s", _("The puzzle you have entered..."), _("Please enter...")); @@ +306,3 @@ + error_str = _("The puzzle you have entered is not a valid Sudoku.\nPlease enter a valid puzzle."); + else + error_str = _("The puzzle you have entered has multiple solutions.\nPlease enter a valid puzzle."); Translator comment here too. But instead of "Please enter a valid puzzle" I would say "Valid Sudoku puzzles have exactly one solution" to clarify why we're not accepting it. Up to you whether to add a "Play Anyway" button. I think the game will work fine regardless, so we don't really need to completely forbid it, just make sure it doesn't happen by accident. @@ +313,3 @@ + }); + + dialog.show (); Is there a reason you needed to use dialog.show() and return to the main loop here? Sometimes you really do need to, but I don't think it's necessary here. I prefer to just call dialog.run() and then dialog.destroy().
(In reply to Michael Catanzaro from comment #29) > > ::: src/gnome-sudoku.vala > @@ +304,3 @@ > + var error_str = ""; > + if (solutions == 0) > + error_str = _("The puzzle you have entered is not a valid > Sudoku.\nPlease enter a valid puzzle."); > > OK, but a couple of i18n thoughts: > > * Please add a translator comment on the line above. > > @@ +306,3 @@ > + error_str = _("The puzzle you have entered is not a valid > Sudoku.\nPlease enter a valid puzzle."); > + else > + error_str = _("The puzzle you have entered has multiple > solutions.\nPlease enter a valid puzzle."); > > Translator comment here too. Do we need to explain something to the translators here?
Yes, all translatable strings should have a comment one line above. (It's not always the case in existing code, but at least we should make sure it's done for new strings.) Just say where the string is used, e.g. something like: /* Warning dialog when starting a custom game if the puzzle is invalid. */ error_str = ...;
Created attachment 302063 [details] [review] Add validity and uniqueness check to manually created puzzles (In reply to Michael Catanzaro from comment #29) > Review of attachment 301889 [details] [review] [review]: > ::: lib/sudoku-board.vala > @@ +395,3 @@ > + public int count_solutions_limited () > + { > + int[] puzzle = new int[rows * cols]; > > I guess it's not possible to use SudokuBoard.cells here, correct? Thanks for pointing this out. Typecasting SudokuBoard.cells to a 1-D array does the job. I've added the 'Play Anyway' button to the multiple solutions dialog. This is how it looks now - http://i.imgur.com/0vTt4wZ.png This patch incorporates all your other suggestions as well :)
Review of attachment 302063 [details] [review]: Thanks! OK to push with changes: ::: lib/qqwing-wrapper.h @@ +29,3 @@ int *qqwing_generate_puzzle(int difficulty); +int qqwing_count_solutions_limited(int* puzzle); +void qqwing_print_stats(int* puzzle); Nit of all nits: since this is a C header rather than a C++ header, I actually put the *s on the variable name intentionally. :) ::: lib/qqwing.vapi @@ +24,3 @@ [CCode (array_length=false)] int[] generate_puzzle (int difficulty); + int count_solutions_limited ([CCode (array_length = false)] int[]puzzle); Nit: space after the [] ::: lib/sudoku-board.vala @@ +395,3 @@ + public int count_solutions_limited () + { + return QQwing.count_solutions_limited ((int[]) cells); Heh, I'm a bit surprised that worked. Cool. :D ::: src/gnome-sudoku.vala @@ +313,3 @@ + var dialog = new MessageDialog (window, DialogFlags.MODAL, MessageType.WARNING, ButtonsType.NONE, warning_str); + dialog.add_button (_("Play _Anyway"), Gtk.ResponseType.ACCEPT); + dialog.add_button (_("_Back"), Gtk.ResponseType.REJECT); These should be reversed: the affirmative button should be to the right of the cancel button. So just flip the two lines.
Review of attachment 302063 [details] [review]: ::: src/gnome-sudoku.vala @@ +295,3 @@ { + int solutions = game.board.count_solutions_limited (); + if (solutions == 1) { One more request: bug #748252 just came in, which finally cleans up our brackets, let's move the bracket after the if statement to the next line. @@ +316,3 @@ + + dialog.response.connect ((response_id) => { + if (response_id == Gtk.ResponseType.ACCEPT) { Bracket on the next line after the if statement here as well. (But not for the lambda, where we do put it on the same line.)
Review of attachment 302063 [details] [review]: Pushed with changes [1]. [1] https://git.gnome.org/browse/gnome-sudoku/commit/?id=c5ce94ea6485740bc9e8b9a9c669cf789042822c