After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 633464 - Rewrite sudoku in vala
Rewrite sudoku in vala
Status: RESOLVED FIXED
Product: gnome-sudoku
Classification: Applications
Component: general
git master
Other Linux
: High enhancement
: ---
Assigned To: gnome-sudoku-maint
gnome-sudoku-maint
Depends on:
Blocks: 625444 710624 710626
 
 
Reported: 2010-10-29 13:47 UTC by Robert Ancell
Modified: 2014-06-04 14:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
toggle_fullscreen_cb: fullscreen item connected. (1.19 KB, patch)
2011-04-07 00:32 UTC, Tiffany Antopolski
committed Details | Review
toggle_toolbar_cb: show toolbar item connected (1.80 KB, patch)
2011-04-07 23:18 UTC, Tiffany Antopolski
committed Details | Review
fullscreen and show toolbar settings saved to gsettings. (5.87 KB, patch)
2011-04-10 08:06 UTC, Tiffany Antopolski
committed Details | Review
Fix computing scale in SudokuCellView (1.11 KB, patch)
2011-06-28 14:27 UTC, Lubomír Sedlář
committed Details | Review
SudokuCellView: compute scale based on height (1.19 KB, patch)
2011-06-28 14:31 UTC, Lubomír Sedlář
committed Details | Review
SudokuCellView: connect signals to number picker (1.36 KB, patch)
2011-06-28 14:33 UTC, Lubomír Sedlář
committed Details | Review
Fix grid stretching problem (861 bytes, patch)
2011-07-03 11:36 UTC, Lubomír Sedlář
committed Details | Review
Add padding to cells (2.10 KB, patch)
2011-07-11 17:02 UTC, Lubomír Sedlář
committed Details | Review
Display sample text as notes (1.30 KB, patch)
2011-07-11 17:08 UTC, Lubomír Sedlář
committed Details | Review
Display note editor (3.07 KB, patch)
2011-07-11 17:17 UTC, Lubomír Sedlář
committed Details | Review
Allow editing notes (2.24 KB, patch)
2011-07-11 17:20 UTC, Lubomír Sedlář
committed Details | Review
Connect signals for clear_top/bottom_notes (1.10 KB, patch)
2011-07-11 17:22 UTC, Lubomír Sedlář
committed Details | Review
Always save notes when hiding editing entry (1.73 KB, patch)
2011-07-12 17:27 UTC, Lubomír Sedlář
committed Details | Review
Add ability to reset game (953 bytes, patch)
2011-08-10 14:46 UTC, Lubomír Sedlář
committed Details | Review
Add undo/redo functionality (4.06 KB, patch)
2011-08-13 09:30 UTC, Lubomír Sedlář
committed Details | Review
Code for solver and new, faster board representation class ( SudokuBoard ). (11.94 KB, patch)
2012-01-18 20:54 UTC, PioneerAxon
needs-work Details | Review
New solver class, SudokuBoard class & their connection to Frontend (27.15 KB, patch)
2012-01-24 05:26 UTC, PioneerAxon
needs-work Details | Review
New solver class, SudokuBoard class & their connection to Frontend, Undo-redo, Hint (31.55 KB, patch)
2012-01-30 06:55 UTC, PioneerAxon
committed Details | Review
Handle incorrect inputs, Gdk keybind to detect key (10.20 KB, patch)
2012-03-18 20:28 UTC, PioneerAxon
committed Details | Review
Front-end bugfix (4.85 KB, patch)
2012-03-25 22:00 UTC, PioneerAxon
committed Details | Review
UI support for multi-sized sudoku, minor bugfix in number_picker (10.14 KB, patch)
2012-04-02 07:25 UTC, PioneerAxon
committed Details | Review
Implemented printing single sudoku feature (10.27 KB, patch)
2013-11-12 15:24 UTC, Parin Porecha
needs-work Details | Review
Updated patch of implemtation of printing single sudoku feature (9.04 KB, patch)
2013-11-12 20:46 UTC, Parin Porecha
committed Details | Review
Updated patch for thickening of sudoku 3x3 block borders (1.04 KB, patch)
2013-11-12 20:55 UTC, Parin Porecha
committed Details | Review
Implemented printing multiple sudokus feature (36.38 KB, patch)
2013-11-21 20:16 UTC, Parin Porecha
needs-work Details | Review
Implemented printing multiple sudokus feature (40.40 KB, patch)
2013-11-24 18:35 UTC, Parin Porecha
committed Details | Review
Patch to fix Sudoku crash on starting Hard/Very Hard games (699 bytes, patch)
2014-02-27 17:10 UTC, Parin Porecha
none Details | Review
Patch to fix GLib critical error related to Sudoku Timer (1.06 KB, patch)
2014-02-27 17:17 UTC, Parin Porecha
none Details | Review
Updated patch to fix Sudoku crash on starting Hard/Very Hard games (1.13 KB, patch)
2014-02-28 13:37 UTC, Parin Porecha
committed Details | Review
Updated patch to fix GLib critical error related to Sudoku Timer (1.21 KB, patch)
2014-02-28 13:38 UTC, Parin Porecha
committed Details | Review
Patch to remove the bug entries in TODO for which I've attached above patches (1.08 KB, patch)
2014-02-28 13:40 UTC, Parin Porecha
committed Details | Review
Add ability to save a game (20.58 KB, patch)
2014-05-05 16:12 UTC, Parin Porecha
none Details | Review
Updated patch to add ability for saving a game (16.42 KB, patch)
2014-05-07 07:40 UTC, Parin Porecha
needs-work Details | Review
Patch which enables 'markasPlayedToggle' and 'includeOldGamesToggle' options in multiple print dialog (8.65 KB, patch)
2014-05-08 11:57 UTC, Parin Porecha
committed Details | Review
Updated SudokuSaver patch (16.79 KB, patch)
2014-05-09 09:54 UTC, Parin Porecha
committed Details | Review
Patch to add ability of generating sudokus (21.87 KB, patch)
2014-05-15 16:29 UTC, Parin Porecha
none Details | Review
Updated patch for Sudoku Generator (19.90 KB, patch)
2014-05-16 04:59 UTC, Parin Porecha
reviewed Details | Review
Move 'Show Possible Numbers' feature to a CLI option (3.94 KB, patch)
2014-05-24 07:42 UTC, Parin Porecha
committed Details | Review
Show 'Clear' button in popover number-picker for a filled cell (3.01 KB, patch)
2014-05-24 08:36 UTC, Parin Porecha
committed Details | Review
Use Popover for number picker and earmarking (14.49 KB, patch)
2014-05-27 11:41 UTC, Parin Porecha
committed Details | Review
Show earmark picker on right-click and Ctrl-Click and show earmarked numbers inline across the top (4.23 KB, patch)
2014-05-28 09:56 UTC, Parin Porecha
accepted-commit_now Details | Review
Show earmark picker on right-click and Ctrl-Click and show earmarked numbers inline across the top (6.33 KB, patch)
2014-05-29 06:53 UTC, Parin Porecha
committed Details | Review
Replace static number picker with New Game & Restart buttons (4.86 KB, patch)
2014-06-01 12:33 UTC, Parin Porecha
reviewed Details | Review
Replace static number picker with New Game & Restart buttons (5.60 KB, patch)
2014-06-02 06:36 UTC, Parin Porecha
committed Details | Review
Show game chooser screen on clicking 'New Game' button (20.11 KB, patch)
2014-06-02 16:19 UTC, Parin Porecha
needs-work Details | Review
Show earmark picker on right-click and Ctrl-Click (1.20 KB, patch)
2014-06-03 01:51 UTC, Michael Catanzaro
committed Details | Review

Comment 1 Robert Ancell 2010-10-29 20:06:01 UTC
This bug is due to the proposal to rewrite glchess in vala (bug #632856).  The above link points to a reasonable start on this.

The advantages I think of porting to Vala are:
- Faster startup time
- We can refactor the current architecture which will make new changes easier (e.g. getting telepathy integrated)

The downsides are:
- The time cost of doing the work
- The potential loss of contributors

The last one I am particularly interested in.  If you have been contributing to Sudoku please let me know if you think this is a good idea.

Also, I don't have the time to rewrite both so please add code to this!!  We can move to GNOME git if that is easier.

In particular, I don't understand the generator/solver so if you know please help out.
Comment 2 Jim Ross 2010-12-08 05:00:45 UTC
Sounds like fun.  I grabbed the repo a week ago or so and started looking at it - and Vala...and bzr.

I think a port could be worthwhile - if for the very least to restructure some of the more fragile architecture decisions in the current app.  It would really be great to design it with telepathy in mind. 

I'll take a stab at the generator and generated game save/retrieve stuff.  What you have so far is certainly a reasonable start.
Comment 3 Robert Ancell 2010-12-08 07:43:34 UTC
+1 for telepathy!

I've just pushed it into the git branch 'sudoku-vala' - in this way it will be easier to keep attributions.  I've got it to compile with GTK3, but there are some rendering bugs (the layout engine has changed in GTK3).

Jim, Not sure if you have a GTK3 dev box, if not I'm happy to migrate patches from the GTK2 version to the GTK3 version.
Comment 4 Jim Ross 2010-12-16 06:52:03 UTC
Well I didn't have a gtk3 box, but now I do.  To the side anyways - I've got a full on bleeding edge environment setup.  Latest gtk3, glib, valac, clutter, etc..  It just took me a bit longer than I thought it would.  I'm working off the latest git version of sudoku with gtk3.  It compiles and runs.

It makes no difference to me which type of repository you use. 

If anyone needs help setting up Ubuntu Maverick Meerkat for the sake of contributing to sudoku, I can certainly help with that. :)  Now to the good stuff - a game generator.  I only have nights, but hopefully it'll go quick.
Comment 5 Thomas Andersen 2011-01-11 21:10:03 UTC
Hi Jim,

how is the port going? Feel free to commit to the git branch if you feel like it.

Gnome3 is not far away. What are the chances of the vala port being ready for it? I can help out if there are specific parts that needs work?
Comment 6 Robert Ancell 2011-03-07 22:37:31 UTC
I am proposing this bug for Google Summer of Code 2011.  I will mentor any successful student.
Comment 7 John Stowers 2011-03-18 01:26:44 UTC
This seems like treading water for no gain? Why port at all? pygobject + g-i is stable and in great shape now.
Comment 8 Jason Clinton 2011-03-19 20:50:48 UTC
Well, we definitely had to port Sudoku to either C, PyGI or Vala so I'm ambivalent about which one it is. Since it's the last Python module in the games suite it probably makes sense to port it to one of either C or Vala.

I'm not sure this makes a good SoC project, though. I'm mostly concerned that it would be too much for an SoC project. I'm not really sure, though.
Comment 9 Johan (not receiving bugmail) Dahlin 2011-03-19 22:54:25 UTC
A PyGTK -> PyGI port will be /way/ less work than porting it to another language. There's a script called pygtk-convert.sh which will automate most of it.
Comment 10 Robert Ancell 2011-03-21 04:48:27 UTC
Johan (not that you appear to be reading this mail :)),
While a PyGTK -> PyGI port will be the least work, it wont have any of the benefits listed in this bug report.  This is a good opportunity to make some new progress.

Jason, I'm confident there is this right amount of work to complete this in the timeframe.  The good thing is, the design is complete (copy the existing Sudoku) and we can always scale the project back if necessary (i.e. there are a number of features to complete, we can drop features if the whole project takes too long).
Comment 11 John Stowers 2011-03-21 08:40:09 UTC
I have observed decreased startup time on all my apps once ported  from PyGTK to pygobject. I also don't think writing in python prevents telepathy integration (there is g-i support in telepathy glib), or makes it harder as is implied. I didn't really see any other benefits on this bug other than 'architectural improvements'.

Anyway, I still think language porting would be a colossal waste of engineering effort better spent elsewhere, but i'm not a gg developer so its not my call.
Comment 12 Thomas Andersen 2011-03-23 00:50:02 UTC
I started working on a port to PyGi a while ago. Even with the few improvements I contributed back to pygi-convert.sh there still seem to be a lot of work to do.

I would personally prefer if we could reduce the number of programming languages used in gnome-games.
Comment 13 John Stowers 2011-03-23 21:51:43 UTC
(In reply to comment #12)
> I started working on a port to PyGi a while ago. Even with the few improvements
> I contributed back to pygi-convert.sh there still seem to be a lot of work to
> do.
> 
> I would personally prefer if we could reduce the number of programming
> languages used in gnome-games.

Can you post this in a branch somewhere please. I'd like to take a look.
Comment 14 Thomas Andersen 2011-03-23 23:59:19 UTC
there is actually already a branch, but please ignore that one. Both sudoku and the pygi-convert script has changed since. I will see if I can replace the old one or create a new one soon.
Comment 15 Robert Ancell 2011-03-26 00:16:35 UTC
For anyone interested on working on this, here are the instructions to play around:

$ git clone http://git.gnome.org/browse/gnome-games
$ cd gnome-games
$ git branch --track sudoku-vala origin/sudoku-vala
$ git checkout sudoku-vala
$ ./autogen.sh --prefix=`pwd`/install
$ cd gnome-sudoku; make; make install
$ ./src/gnome-sudoku

If you make changes, then git commit them and use "git format-patch
origin" and attach the patches to the bug report.

You will need a working GTK3 stack to compile and run.

If you want to play around with the existing Python version:

$ git clone http://git.gnome.org/browse/gnome-games gnome-games-python
$ cd gnome-games-python
$ ./autogen.sh --prefix=`pwd`/install
$ cd gnome-sudoku; make; make install
$ PYTHONPATH=`pwd`/../install/lib/python2.4/site-packages/ ./src/gnome-sudoku
Comment 16 Robert Ancell 2011-03-26 01:10:43 UTC
Vala tutorial:
https://live.gnome.org/Vala/Tutorial
Comment 17 Tiffany Antopolski 2011-04-07 00:32:55 UTC
Created attachment 185377 [details] [review]
toggle_fullscreen_cb: fullscreen item connected.
Comment 18 Tiffany Antopolski 2011-04-07 23:18:59 UTC
Created attachment 185487 [details] [review]
toggle_toolbar_cb: show toolbar item connected
Comment 19 John Stowers 2011-04-07 23:20:34 UTC
The port to python gobject-introspection is currently occurring in master BTW.
Comment 20 Robert Ancell 2011-04-08 00:54:18 UTC
Thanks Tiffany, I've applied these to master.  The next step is for the settings to be saved into gsettings (See the glchess code to see the same code in action).
Comment 21 Robert Ancell 2011-04-08 01:48:33 UTC
John, the bug about the GI work is bug #626227.  Note that this is still a planned feature for 3.2 so all contributions are still requested and welcome!
Comment 22 Robert Ancell 2011-04-08 01:51:44 UTC
Review of attachment 185487 [details] [review]:

Works well.  The setting should be saved to gsettings, see the glchess code for how to do this.
Comment 23 Robert Ancell 2011-04-08 01:52:19 UTC
Review of attachment 185377 [details] [review]:

Works well.  As you have noted, calling fullscreen doesn't actually guarantee the window will go fullscreen.  Set glchess code for how to handle this.
Comment 24 John Stowers 2011-04-08 02:37:09 UTC
(In reply to comment #21)
> John, the bug about the GI work is bug #626227.  Note that this is still a
> planned feature for 3.2 so all contributions are still requested and welcome!

Whats the point?

I thought that Thomas (whom I assumed was the owner of soduku) committing to master was a sign that we will go the pygi way.

"Reassigned to gnome-sudoku as this is the last module that uses PyGTK.  Note
that Sudoku is being rewritten in Vala (bug #633464) which will probably be
completed in time for 3.2."

Do we really have such an excess of developers that one of these ports will go unused?

Please be clear - I would rather be disappointed than have my time wasted.
Comment 25 Robert Ancell 2011-04-08 04:16:27 UTC
Thomas is a GNOME Games maintainer and I am a GNOME Games developer.  We do not have formal owners for each of the games.

When I started porting Chess to Vala (it is a very old codebase, the architecture is inappropriate and a maintenance burden) Thomas requested: (Bug #632856)

"+1 (but only if this is done for both games)" (referring to Sudoku)

We had an existing bug open about porting to PyGI (Bug #626227) in which Thomas states:

"Long term plans still involve moving to vala but we need something for 3.0"

GNOME Games was released with three games disabled, as they did not work in GNOME3 (Sudoku, Swell Foop, Lights Off).  Swell Foop and Lights Off were recently fixed for the 3.0.1 release.  I'll leave it for Thomas to confirm, but I expect the work he and you are doing will be used for the 3.0.1 release to re-enable Sudoku.

To be 100% clear:
- Sudoku is being ported to Vala
- This will not be completed in the 3.0 release, it is scheduled for 3.2
- This is not a waste of developer resource.  The GNOME games project does not have a developer budget, or even a finite amount of developer time.  Like all open source projects developer time is only limited by interest and ability and anyone working on this bug is not taking away from any other work.

Please keep further discussion relating to a GI port of GNOME Sudoku in bug #626227 as these comments are generating confusion with summer of code students.
Comment 26 Robert Ancell 2011-04-08 04:52:40 UTC
To clarify a question I was asked outside of bugzilla: I am responsible for this being implemented.  I will mentor students and new contributors to get the work done but I will complete the work if we don't get enough resource.
Comment 27 Tiffany Antopolski 2011-04-10 08:06:39 UTC
Created attachment 185629 [details] [review]
fullscreen and show toolbar settings saved to gsettings.


With regards to the fullscreen functionality:
    If I close the program in fullscreen mode, then open it, it seems to only toggle after the third time of clicking Settings-->Fullscreen.  Then, after toggling four more times, the grid elongates vertically.
    However, if you close the program in fullscreen mode, then open it, Alt-Tab away from it, then return to it, then the toggling happens as expected (minus the grid elongation).
    Something's messed up...
Comment 28 Allison Karlitskaya (desrt) 2011-04-11 02:11:34 UTC
(In reply to comment #24)
> Whats the point?
> 
> I thought that Thomas (whom I assumed was the owner of soduku) committing to
> master was a sign that we will go the pygi way.

Please see this email:

http://mail.gnome.org/archives/games-list/2011-April/msg00004.html
Comment 29 Robert Ancell 2011-04-11 03:57:07 UTC
Review of attachment 185629 [details] [review]:

Great patch!  I know you were looking for feedback so I really scrutinised it for mistakes.  The only thing I could come up with were trivial whitespace changes (one part had 8 spaces instead on 4 and the EXTRA_DIST wasn't set the same as the other variables).
Comment 30 Robert Ancell 2011-04-11 03:59:05 UTC
Regarding the grid stretching - I get the same bug.  I think it's due to something with GTK3, perhaps the grid code isn't setting something correctly.  It did work fine when I had it running under GTK2.
Comment 31 Lubomír Sedlář 2011-06-28 14:27:28 UTC
Created attachment 190866 [details] [review]
Fix computing scale in SudokuCellView
Comment 32 Lubomír Sedlář 2011-06-28 14:31:33 UTC
Created attachment 190868 [details] [review]
SudokuCellView: compute scale based on height

Since digits tend to be higher than wider, scaling them to fit vertically makes more sense.
Comment 33 Lubomír Sedlář 2011-06-28 14:33:18 UTC
Created attachment 190869 [details] [review]
SudokuCellView: connect signals to number picker
Comment 34 Robert Ancell 2011-06-30 07:32:24 UTC
Review of attachment 190868 [details] [review]:

Thanks!
Comment 35 Robert Ancell 2011-06-30 07:32:36 UTC
Review of attachment 190869 [details] [review]:

Thanks!
Comment 36 Robert Ancell 2011-06-30 07:33:01 UTC
Review of attachment 190866 [details] [review]:

Thanks!
Comment 37 Lubomír Sedlář 2011-07-03 11:36:51 UTC
Created attachment 191180 [details] [review]
Fix grid stretching problem

This is more of a bypassing than proper fix, but it should get the work done.
Comment 38 Robert Ancell 2011-07-06 00:49:41 UTC
Review of attachment 191180 [details] [review]:

Works well, applied thanks!
Comment 39 Lubomír Sedlář 2011-07-11 17:02:07 UTC
Created attachment 191739 [details] [review]
Add padding to cells

This padding can be used to display notes and it makes the cells look less cluttered.

It is implemented in get_preferred_width/height by multiplying the computed size with a constant.
Comment 40 Lubomír Sedlář 2011-07-11 17:08:16 UTC
Created attachment 191743 [details] [review]
Display sample text as notes
Comment 41 Lubomír Sedlář 2011-07-11 17:17:10 UTC
Created attachment 191744 [details] [review]
Display note editor

This patch adds the note editor. The position of the window should be correct. I could not manage to make the entry smaller, though. Once that Gtk+ 3.2 is out, it might be good to replace popup window with GtkOverlay widget.
http://developer.gnome.org/gtk3/3.1/GtkOverlay.html
Comment 42 Lubomír Sedlář 2011-07-11 17:20:23 UTC
Created attachment 191745 [details] [review]
Allow editing notes

This patch also changes key press handling to key_press_event instead of release, as that caused unwanted displaying of number picker when note editor was hidden by activate signal on entry.
Comment 43 Lubomír Sedlář 2011-07-11 17:22:54 UTC
Created attachment 191746 [details] [review]
Connect signals for clear_top/bottom_notes
Comment 44 Robert Ancell 2011-07-11 23:35:35 UTC
Review of attachment 191739 [details] [review]:

Looks good, applied.
Comment 45 Robert Ancell 2011-07-11 23:41:27 UTC
Review of attachment 191743 [details] [review]:

Committed, works well.   You probably could have combined all the notes patches into one, but not a big deal.  One issue I've noticed is if you click outside the note editor is reverts your changes instead of applying them.

Great work Lubomír!  Keep going! :)
Comment 46 Lubomír Sedlář 2011-07-12 17:27:33 UTC
Created attachment 191827 [details] [review]
Always save notes when hiding editing entry
Comment 47 Lubomír Sedlář 2011-08-10 14:46:32 UTC
Created attachment 193556 [details] [review]
Add ability to reset game
Comment 48 Lubomír Sedlář 2011-08-13 09:30:50 UTC
Created attachment 193748 [details] [review]
Add undo/redo functionality

I added before_value_changed signal to SudokuCell, so that it is possible to record the value before it changes.

Both undo and redo stack are stored as singly linked list (GLib.SList) of UndoItem, which is a struct containing row, column and value of changed cell. Any time a cell is changed, the previous value is stored in undo stack and redo stack is emptied. On undo action the first item is removed from the list, the cell changes its value to the recorded one and the same structure is pushed to redo stack. On redo the similar action is performed, only the lists are swapped.

When reset action is activated, many items are stored in the undo stack, the last one of them with both row and column of -1. The value is set to the number of reset cells. On undo of such operation, appropriate number of changes is undone (so that whole reset action is undone with single click on Undo button).
Comment 49 Robert Ancell 2011-12-19 10:47:16 UTC
Review of attachment 191827 [details] [review]:

Wasn't sure what this patch was fixing but looks safe so pushed
Comment 50 Robert Ancell 2011-12-19 10:47:40 UTC
Review of attachment 193556 [details] [review]:

Works great!
Comment 51 Robert Ancell 2011-12-19 10:48:13 UTC
Review of attachment 193748 [details] [review]:

Works great!  I pushed it with some minor whitespace changes to match the existing code.
Comment 52 Robert Ancell 2011-12-19 10:49:39 UTC
Hi Lubomír,  sorry I completely missed these patches were here.  If you don't get any responses please feel free to ping me (IRC, email) or another GNOME Games developer and we will do our best to review these faster.
Comment 53 PioneerAxon 2012-01-18 20:54:56 UTC
Created attachment 205583 [details] [review]
Code for solver and new, faster board representation class ( SudokuBoard ).
Comment 54 Robert Ancell 2012-01-22 06:46:13 UTC
Review of attachment 205583 [details] [review]:

Great start!  The solver looks good, and seems to work well.

Some things to fix up:
- Make sure to match the existing style of the code that's being modified.  So whitespace, naming conventions etc should be the same.
- The patch shouldn't contain any dead / commented out code.  If you've disabled existing code delete it from the patch, if there's code that's being worked / that will need adding in the future add a /* TODO: ... */ style comment.
- The new solver code should replace the existing code - so SudokuBoard should be merged into SudokuGame and connect up to the SudokuView.

Keep up the good work!
Comment 55 PioneerAxon 2012-01-24 05:26:46 UTC
Created attachment 205951 [details] [review]
New solver class, SudokuBoard class & their connection to Frontend

Sorry for this gigantic ( 27K+ ) patch, I had to connect every class with new representation & update almost every class.
Comment 56 Robert Ancell 2012-01-24 05:37:59 UTC
Review of attachment 205951 [details] [review]:

Looks generally good based on a quick review.  The three things listed as broken will need to be fixed before committing.  The SudokuBoard class should be renamed to SudokuGame (or you should add a Game class that contains a board (or stack of boards for undo, tracker behaviour).  This is because the sudoku-game.vala file should contain a SudukuGame class.
Comment 57 PioneerAxon 2012-01-30 06:55:03 UTC
Created attachment 206396 [details] [review]
New solver class, SudokuBoard class & their connection to Frontend, Undo-redo, Hint

Again sorry for this gigantic patch..
Comment 58 Robert Ancell 2012-01-31 04:24:27 UTC
Review of attachment 206396 [details] [review]:

OK, Applied with a following patch that fixes up some coding style issues.  There is one issue with this change in that it stops you from entering invalid values into the cells - I tried disabling this but it caused it to segfault.  This will need to be fixed in a following patch.
Comment 59 PioneerAxon 2012-03-18 20:28:15 UTC
Created attachment 210060 [details] [review]
Handle incorrect inputs, Gdk keybind to detect key

SudokuBoard now handles wrong inputs by saving status bit saying broken or not. On update or remove, it automatically updates status and possible flags.
key_press_event now uses Gdk.keyval_name to detect the value of key, instead of keyval.
added key_map_kaypad to map keypad.
Minor bugfix in Hint function, fixed: range of random number generator.
Minor bugfix in apply_stack, fixed: removed multiple insert on val = 0 in stack.
Comment 60 PioneerAxon 2012-03-25 22:00:02 UTC
Created attachment 210594 [details] [review]
Front-end bugfix

Patch to fix pop-ups not getting keyboard input focus.
Both note-editor and number-picker now grabs focus properly.
Comment 61 Robert Ancell 2012-03-28 03:26:15 UTC
Review of attachment 210060 [details] [review]:

Looks good
Comment 62 Robert Ancell 2012-03-28 03:26:29 UTC
Review of attachment 210594 [details] [review]:

Looks good
Comment 63 Robert Ancell 2012-03-28 04:05:56 UTC
Note I just merged the gnome-sudoku branch with master as we've diverged a lot.  You may need to re-run autogen.sh after doing a git pull.
Comment 64 PioneerAxon 2012-03-28 21:57:56 UTC
I tried to re-run autogen.sh , but it was unable to complete.

It gave me error saying

libgames-support/Makefile.am:89: HAVE_INTROSPECTION does not appear in AM_CONDITIONAL

If you want, full dump is on http://pastebin.com/RXpuH3x4
Comment 65 Robert Ancell 2012-03-29 01:39:19 UTC
I just did a fresh clone of the repository and it works, so it's probably just autotools being confused.

Sometimes this fixes it:
$ make distclean
$ rm -r autom4te.cache
$ find . -name '.deps' | xargs rm -r

Then you should be back to a clean slate.
Comment 66 PioneerAxon 2012-03-29 10:03:43 UTC
I solved that problem by installing following packages. I think, former version didn't need these packages, and are new..

gobject-introspection
valac-0.16
valac-0.16-dbg

I am still unable to compile it because of older version of GTK 3.

checking for GTK... no
configure: error: Package requirements (gtk+-3.0 >= 3.3.11) were not met:

Requested 'gtk+-3.0 >= 3.3.11' but version of GTK+ is 3.2.4

So, I will have to get gtk+ 3.4 installed from source code, as it is not available on ubuntu 11.10. :(
Comment 67 Robert Ancell 2012-03-29 21:59:03 UTC
PioneerAxon: Try configuring with --enable-games=gnome-sudoku - that should drop the GTK+ requirements to 3.0.0
Comment 68 PioneerAxon 2012-03-30 02:54:44 UTC
It worked. But, now I am getting error at make.

  CCLD   gnome-sudoku
/bin/bash: ../../libtool: No such file or directory
make[1]: *** [gnome-sudoku] Error 127
make[1]: Leaving directory `/home/####/Desktop/gnome/gnome-games/gnome-sudoku/src'
make: *** [all-recursive] Error 1

there is libtoolT at that location. Don't know what is missing. I have libtool installed.
Comment 69 PioneerAxon 2012-03-30 07:13:41 UTC
Finally got it running.. 

Had to rename "libtoolT" file to "libtool" in the root directory, i.e. gnome-games.

Not sure if there is a bug in makefile generation or is there something wrong with my environment.


Anyways, Thanks a lot for getting me back on track..
Comment 70 PioneerAxon 2012-04-01 18:49:11 UTC
I just had to switch to Ubuntu 12.04 for some reason. I now can compile everything without any problem. But I don't seem to be able to run it.


arth@arth-HP-DM1:~/Development/gnome-games/gnome-games/gnome-sudoku$ ./src/gnome-sudoku 

(gnome-sudoku:5797): GLib-GIO-ERROR **: Settings schema 'org.gnome.gnome-sudoku' does not contain a key named 'fullscreen'
Trace/breakpoint trap (core dumped)


I think, the schema is installed properly. Or is it missing?
Comment 71 Robert Ancell 2012-04-01 23:38:42 UTC
You have to set the environment so that the current gsettings schema is being used (this needs to be done for any application that uses a changed gsettings schema):
XDG_DATA_DIRS=`pwd`/install/share:$XDG_DATA_DIRS ./gnome-sudoku/src/gnome-sudoku
Comment 72 PioneerAxon 2012-04-02 02:39:35 UTC
Robert Ancell, Thanks.. I finally got it running.
Some patches will be coming soon.
Comment 73 PioneerAxon 2012-04-02 07:25:11 UTC
Created attachment 211110 [details] [review]
UI support for multi-sized sudoku, minor bugfix in number_picker
Comment 74 Parin Porecha 2013-11-10 11:00:07 UTC
Hi,

I want to contribute to the vala-port. I think the print section might be a good point of entry, since no work has been done on it.
I'll start with printing single sudoku (Printing multiple ones requires creating a custom UI dialog, so that'll be done once I get this working)

Developers, do you have a better/higher-priority suggestion which you want to get ported before the printing part ?
Comment 75 Michael Catanzaro 2013-11-11 14:07:04 UTC
That sounds like a good place to start to me, thanks!
Comment 76 Parin Porecha 2013-11-12 15:24:48 UTC
Created attachment 259669 [details] [review]
Implemented printing single sudoku feature

I've attached the patch for the printing single sudoku feature. I have one question -

Which looks better ? -
http://i.imgur.com/1VRsFtD.png
http://i.imgur.com/7wTJXW2.png

Both of them have very subtle changes. The first one does not have much contrast between thick lines (between blocks) and thin lines (inside blocks). It is the current print version used in master branch.
Since in vala-port, sudokus in the game have more contrast, should we keep the same while printing ?

I've kept the settings to print sudoku like the one in 2nd image. But, I'm not a dev, so it's definitely your call. If you have a better setting for border thickness, I'll change it.

Here are the details of the change -
In src/sudoku-printer.vala,
106        double THIN = size / 500.0;       // changed from 300.0 to 500.0
107        double THICK = THIN * 5;         // changed from 2 to 5
Comment 77 Michael Catanzaro 2013-11-12 17:05:14 UTC
Review of attachment 259669 [details] [review]:

Looks pretty good overall, thanks!

Regarding sudoku-printer.vala: we don't normally use the "this" keyword except where necessary.  There's nothing wrong with doing so, it's just not the style we use.

::: src/gnome-sudoku.vala
@@ +37,3 @@
 
+    // Printer
+    private SudokuPrinter printer;

It's good to declare variables with the smallest scope necessary.  Since you only use this one within the conditional in print_cb, it should go there.  (You can actually declare it and initialize it in the same line.)

This is also (very) slightly cleaner with regards to memory use, since the printer object won't be kept around after it's no longer needed.

::: src/sudoku-printer.vala
@@ +6,3 @@
+    public SudokuBoard board;
+    private DifficultyRating difficulty_rating;
+    public ApplicationWindow window;

We don't (or shouldn't) use public fields.  Since you're not using them anywhere outside of the class, you can just make them private.

@@ +33,3 @@
+        this.margin = 25;
+        this.n_sudokus = 1;
+        this.sudokus_per_page = 1;

I guess these will no longer be constants when it's upgraded for multiple sudokus, correct?  If so, this is fine.

@@ +96,3 @@
+    private void draw_sudoku (Cairo.Context cr, double size, int offset_x, int offset_y)
+    {
+        int SUDOKU_SIZE = 9;

These should all be declared as const.

@@ +105,3 @@
+
+        double THIN = size / 500.0;
+        double THICK = THIN * 5;

I think these values look good.

::: src/sudoku-view.vala
@@ +668,3 @@
                 cell.show ();
 
+                if (col != 0 && (col % game.board.block_cols) == 0)

Could you please remove these fixes from this patch and attach a separate patch for them?  It's hard to keep the project history readable when changes sneak into unrelated patches.
Comment 78 Parin Porecha 2013-11-12 20:46:49 UTC
(In reply to comment #77)
> Review of attachment 259669 [details] [review]:
> 
> Looks pretty good overall, thanks!
> 
> Regarding sudoku-printer.vala: we don't normally use the "this" keyword except
> where necessary.  There's nothing wrong with doing so, it's just not the style
> we use.

Okay, I've changed them.

> ::: src/gnome-sudoku.vala
> @@ +37,3 @@
> 
> +    // Printer
> +    private SudokuPrinter printer;
> 
> It's good to declare variables with the smallest scope necessary.  Since you
> only use this one within the conditional in print_cb, it should go there.  (You
> can actually declare it and initialize it in the same line.)
> 
> This is also (very) slightly cleaner with regards to memory use, since the
> printer object won't be kept around after it's no longer needed.
> 
> ::: src/sudoku-printer.vala
> @@ +6,3 @@
> +    public SudokuBoard board;
> +    private DifficultyRating difficulty_rating;
> +    public ApplicationWindow window;
> 
> We don't (or shouldn't) use public fields.  Since you're not using them
> anywhere outside of the class, you can just make them private.

Done

> @@ +33,3 @@
> I guess these will no longer be constants when it's upgraded for multiple
> sudokus, correct?  If so, this is fine.

Yes, they are constants just till I implement multiple printing.

> @@ +96,3 @@
> +    private void draw_sudoku (Cairo.Context cr, double size, int offset_x, int
> offset_y)
> +    {
> +        int SUDOKU_SIZE = 9;
> 
> These should all be declared as const.

Done. The ones depending on parameter 'size' are left unchanged.

> @@ +105,3 @@
> +
> +        double THIN = size / 500.0;
> +        double THICK = THIN * 5;
> 
> I think these values look good.
> 
> ::: src/sudoku-view.vala
> @@ +668,3 @@
>                  cell.show ();
> 
> +                if (col != 0 && (col % game.board.block_cols) == 0)
> 
> Could you please remove these fixes from this patch and attach a separate patch
> for them?  It's hard to keep the project history readable when changes sneak
> into unrelated patches.

Removed them.

Michael > Thanks for taking the time to explain these little details. I use Python, so I'm learning Vala alongside. I apologize for these syntax and code-style errors. I'll remember them, so that they don't get repeated.
Comment 79 Parin Porecha 2013-11-12 20:46:55 UTC
Created attachment 259693 [details] [review]
Updated patch of implemtation of printing single sudoku feature

> Regarding sudoku-printer.vala: we don't normally use the "this" keyword except
> where necessary.  There's nothing wrong with doing so, it's just not the style
> we use.

Okay, I've changed them.

> ::: src/gnome-sudoku.vala
> @@ +37,3 @@
> 
> +    // Printer
> +    private SudokuPrinter printer;
> 
> It's good to declare variables with the smallest scope necessary.  Since you
> only use this one within the conditional in print_cb, it should go there.  (You
> can actually declare it and initialize it in the same line.)
> 
> This is also (very) slightly cleaner with regards to memory use, since the
> printer object won't be kept around after it's no longer needed.
> 
> ::: src/sudoku-printer.vala
> @@ +6,3 @@
> +    public SudokuBoard board;
> +    private DifficultyRating difficulty_rating;
> +    public ApplicationWindow window;
> 
> We don't (or shouldn't) use public fields.  Since you're not using them
> anywhere outside of the class, you can just make them private.

Done

> @@ +33,3 @@
> I guess these will no longer be constants when it's upgraded for multiple
> sudokus, correct?  If so, this is fine.

Yes, they are constants just till I implement multiple printing.

> @@ +96,3 @@
> +    private void draw_sudoku (Cairo.Context cr, double size, int offset_x, int
> offset_y)
> +    {
> +        int SUDOKU_SIZE = 9;
> 
> These should all be declared as const.

Done. The ones depending on parameter 'size' are left unchanged.

> ::: src/sudoku-view.vala
> @@ +668,3 @@
>                  cell.show ();
> 
> +                if (col != 0 && (col % game.board.block_cols) == 0)
> 
> Could you please remove these fixes from this patch and attach a separate patch
> for them?  It's hard to keep the project history readable when changes sneak
> into unrelated patches.

Removed them.

Michael > Thanks for taking the time to explain these little details. I use Python, so I'm learning Vala alongside. I apologize for these syntax and code-style errors. I'll remember them, so that they don't get repeated.
Comment 80 Parin Porecha 2013-11-12 20:55:30 UTC
Created attachment 259694 [details] [review]
Updated patch for thickening of sudoku 3x3 block borders

Attached a separate patch for thickening of 3x3 block borders.

And, sorry for the above twice posted comment
Comment 81 Michael Catanzaro 2013-11-12 22:06:00 UTC
That's fine, thanks for your patches!

Attachment 259693 [details] pushed as 8acd021 - Updated patch of implemtation of printing single sudoku feature
Attachment 259694 [details] pushed as 5cf8280 - Updated patch for thickening of sudoku 3x3 block borders
Comment 82 Parin Porecha 2013-11-14 15:54:07 UTC
I've started working on printing multiple sudokus and I've some questions -

- Master branch uses print_games.ui file which contains preferences for printing multiple ones. Can I use the file in vala-port branch ?


- New sudokus are generated by selecting one sudoku randomly out of several from data files. These files contain a string like -
0 0 0 3 6 0 9 2 7 0 0 9 5 0 0 0 3 8 0 0 0 0 9 8 0 0 5 0 0 0 0 0 0 0 9 6 5 0 4 0 2 0 7 0 3 9 3 0 0 0 0 0 0 0 8 0 0 7 3 0 0 0 0 7 9 0 0 0 5 8 0 0 4 1 5 0 8 6 0 0 0	0.753472222222

I believe the last float represents the difficulty rating ?
Now, I'll need to get the ratings for each sudoku to be printed.

Master branch uses the float to get the rating. However, vala-port does not. (Why?)
vala-port just reads till character 162 and generates sudoku from that.
To get the rating for a board, currently, we call get_difficulty() which calls solve() (Why do we solve a board to get the rating when we can read it from the file itself ?)
So, I propose that we read the float from the file and save it for each board while generating.

Should I go forward with it ?(I'm asking because I found it odd why the easy way wasn't followed in vala-port)
Comment 83 Michael Catanzaro 2013-11-16 22:34:19 UTC
(In reply to comment #82)
> I've started working on printing multiple sudokus and I've some questions -
> 
> - Master branch uses print_games.ui file which contains preferences for
> printing multiple ones. Can I use the file in vala-port branch ?

You should be able to, yes. That would save a lot of work.

> Master branch uses the float to get the rating. However, vala-port does not.
> (Why?)

So the Vala version saves the float but does not use it (if so, it should use it), or does it neither save or use it (and the old data files still happen to have it)?

The Vala port is pretty rough so this may just be an oversight, but maybe there's a reason for it. (Chris, hi, any thoughts on this?)  E.g. computing it on the fly allows us to change the difficulty levels of generated puzzles in future versions. But if computing a bunch of difficulty levels all at once slows down Print Multiple, then we should probably store them in the file.
Comment 84 Parin Porecha 2013-11-17 00:53:43 UTC
(In reply to comment #83)
> So the Vala version saves the float but does not use it (if so, it should use
> it), or does it neither save or use it (and the old data files still happen to
> have it)?

Vala version does not even read the float. It reads a line only till the point necessary to generate a board from it
In sudoku-store.vala -
21     board.set_from_string(line[0:161], " ");     // The float starts from character 162

When rating is needed, the board is cloned and sent to solve
In sudoku-solver.vala -
577    public DifficultyRating get_difficulty () {
578        if (!solved)
579            solve();
Printing multiple boards may slow down due to this

> The Vala port is pretty rough so this may just be an oversight, but maybe
> there's a reason for it. (Chris, hi, any thoughts on this?)  E.g. computing it
> on the fly allows us to change the difficulty levels of generated puzzles in
> future versions.

Oh okay. I was wondering why the lines in gnome-sudoku.vala using SudokuGenerator class were commented out. It's not complete, it seems.
Till SudokuGenerator becomes fully functional, we will be using SudokuStore for getting new puzzles when requested, right ?
So, in that case, I think reading the rating from the file to avoid the unnecessary computation is the practical thing to do.
Comment 85 Parin Porecha 2013-11-21 20:16:37 UTC
Created attachment 260484 [details] [review]
Implemented printing multiple sudokus feature

I've attached the patch for the printing multiple sudokus feature.

Some clarifications :
- As tracker has not been ported to vala-port yet, the corresponding options ("Include games you've already played", "mark game as played once printed") in the print dialog, have been disabled for now.

- Getting the catagory of a sudoku board (EASY, MEDIUM, ...) was also not possible without create an instance of SudokuRater class (which solved the board to get the rating). As we're now saving the rating read from the file, I've written a method in SudokuBoard class which gets the catagory from rating float itself.

- To get a board, we currently have only get_random_* methods. Because of it, *very rarely* a sudoku might get repeated in multiple printing (I found 1-2 repeats on running the printing >20 times). However, this will get resolved once the port of sudoku-generator.vala is complete.

So, I think printing feature is complete :-)
Comment 86 Michael Catanzaro 2013-11-22 02:14:31 UTC
Review of attachment 260484 [details] [review]:

Great work, Parin, this was a big feature for a new contributor.  Accordingly, I've left a lot more comments than I usually would, but I'm looking forward to merging this once they're addressed.  Also, you mentioned a couple of deficiencies that we need to address later down the road: when you update this patch, could you add those explanations to the TODO file? Thanks a bunch!

One general comment (especially in SudokuPrinter.vala) is to remember to use the var keyword to declare variables wherever possible, as a matter of style.  There's not really consensus as to whether this is a good or bad idea, but that's how the other Vala games were originally written and we want to stick to that style.  For example:

double left = margin; // var left = margin;
double top = margin; // var top = margin;
int[] pos = {1, 1}; // var pos = {1, 1};
string label = ""; // var label = "";

You usually only need to name the data types when declaring fields (member variables) and methods.

::: data/print-games.ui
@@ +1,3 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<interface>
+  <requires lib="gtk+" version="2.16"/>

Could you change the target version to 3.0?  You'll have trouble opening it in modern Glade otherwise.

::: src/gnome-sudoku.vala
@@ +469,3 @@
     {
+        GamePrinter printer = new GamePrinter(sudoku_store, ref window);
+        printer.run_dialog();

Don't forget to leave the space before the parentheses!

::: src/sudoku-board.vala
@@ +64,3 @@
+    public double _difficulty_rating;
+    public double difficulty_rating
+    {

For a trivial getter/setter like this, Vala has a nice one-line syntax so that we can get rid of the _difficulty_rating variable (which should have been declared private). I know this is not consistently used in the Sudoku code, but it'd be good to start doing so.  The shortcut syntax is:

public double difficulty_rating { get; set; }

Or to provide only a public setter:

public double difficulty_rating { get; private set; }

@@ +71,3 @@
+    private bool in_range (float[] range)
+    {
+        return (difficulty_rating >= range[0] && difficulty_rating < range[1]);

Yipes, we'll need to rethink the DifficultyRating range interface... that can be done later, though.

::: src/sudoku-printer.vala
@@ +29,3 @@
     }
 
+    public SudokuPrinter (SudokuBoard[] boards, ref  ApplicationWindow window, int sudokus_per_page = 1)

Nitpick: extra whitespace after ref.

::: src/sudoku-store.vala
@@ +20,3 @@
                     SudokuBoard board = new SudokuBoard();
                     board.set_from_string(line[0:161], " ");
+                    board.difficulty_rating = double.parse(line[162:line.length]);

I think it'd be better to modify set_from_string to operate on the entirety of the line.  That will ensure that it's not possible to forget to set difficulty_rating.  It also allows you to make difficulty_rating private, since there's no reason one should be able to change the difficulty of a SudokuBoard after it's been created.

@@ +118,3 @@
+        int i = 0;
+        int  il = 0;
+        int lev = 0;

I almost didn't write this comment, since this is a nitpick, but we prefer more verbose variable names.  I have an idea below on how we can get rid of "lev."  Then "il" could become "level." i and n are both clear and fine, though.

Also, there's an extra space in the declaration of il.

@@ +123,3 @@
+            levels = {DifficultyCatagory.EASY, DifficultyCatagory.MEDIUM, DifficultyCatagory.HARD, DifficultyCatagory.VERY_HARD};
+
+        while (i < n)

It'd be much cleaner to write this as a for loop.

@@ +128,3 @@
+                il = 0;
+            lev = levels[il];
+            boards.add(get_random_board((DifficultyCatagory) lev));

lev should be removed; you can just do:

boards.add (get_random_board ((DifficultyCatagory) levels[il]));
Comment 87 Parin Porecha 2013-11-24 18:35:24 UTC
Created attachment 261366 [details] [review]
Implemented printing multiple sudokus feature

(In reply to comment #86)
> double left = margin; // var left = margin;
> double top = margin; // var top = margin;
> int[] pos = {1, 1}; // var pos = {1, 1};

'margin' is int, whereas 'left' and 'top' are better as double for precision. I've changed all the rest of the declarations, excluding a few variables similar to 'margin'

> Could you change the target version to 3.0?  You'll have trouble opening it in
> modern Glade otherwise.

Done

> Don't forget to leave the space before the parentheses!

Ah, sorry I forget it!
I found some more of them; changed all.

> For a trivial getter/setter like this, Vala has a nice one-line syntax so that
> we can get rid of the _difficulty_rating variable (which should have been
> declared private).

Thanks for the tip !
Changed it to -
> public double difficulty_rating { get; private set; }

> Nitpick: extra whitespace after ref.

Corrected

> I think it'd be better to modify set_from_string to operate on the entirety of
> the line.  That will ensure that it's not possible to forget to set
> difficulty_rating.  It also allows you to make difficulty_rating private, since
> there's no reason one should be able to change the difficulty of a SudokuBoard
> after it's been created.

Yes, that's a nice idea. Much better than the current way.
I've changed it to -
-                     board.set_from_string(line[0:161], " ");
+                    board.set_from_string(line, " ");
'set_from_string' now handles setting the board and difficulty_rating property.

> @@ +118,3 @@
> +        int i = 0;
> +        int  il = 0;
> +        int lev = 0;
> 
> I almost didn't write this comment, since this is a nitpick, but we prefer more
> verbose variable names.  I have an idea below on how we can get rid of "lev." 
> Then "il" could become "level." i and n are both clear and fine, though.

In fact, I found we don't need 'il' at all. This does the same job and is concise -
  boards.add (get_random_board ((DifficultyCatagory) levels[i++ % levels.length]));

> 
> @@ +123,3 @@
> +            levels = {DifficultyCatagory.EASY, DifficultyCatagory.MEDIUM,
> DifficultyCatagory.HARD, DifficultyCatagory.VERY_HARD};
> +
> +        while (i < n)
> 
> It'd be much cleaner to write this as a for loop.

Yes, it'd be cleaner, but the while loop in 'printing.py' is a little more complicated.
The original while loop was -
  while i < n and len(finished) < len(levels):
where 'finished' list changes dynamically in the loop

The one I've written is -
  while (i < n)
This is because the method 'get_assorted_boards' is currently tailor-made for printing purposes.  It isn't used anywhere else. In the future when more options like boards to exclude have to be added, it'll be easier to just do -
  while (i < n && finished.length < levels.length)
I should've mentioned this before; have done now.
Comment 88 Michael Catanzaro 2013-11-24 22:24:46 UTC
Comment on attachment 261366 [details] [review]
Implemented printing multiple sudokus feature

Neat, thank you!

Attachment 261366 [details] pushed as 19ad23c - Implemented printing multiple sudokus feature
Comment 89 Parin Porecha 2014-02-27 17:10:01 UTC
Created attachment 270494 [details] [review]
Patch to fix Sudoku crash on starting Hard/Very Hard games

Hi,
I am attaching patches for 2 bugs I have found in vala-port (Both are filed in TODO).

This patch is for :
- Sometimes starting a 'Hard' or 'Very Hard' game causes the application to crash with the following error -
ERROR:hashmap.c:3434:gee_hash_map_map_iterator_real_get_key: assertion failed: (_node != null)
Aborted (core dumped)

According to the docs, when the MapIterator is out of track, neither get_key, get_value, set_value nor unset are defined and all will fail. After the next call to next, they will be defined again.
Comment 90 Parin Porecha 2014-02-27 17:17:38 UTC
Created attachment 270496 [details] [review]
Patch to fix GLib critical error related to Sudoku Timer

This patch is for :

- GLib-CRITICAL **: g_timer_continue: assertion 'timer->active == FALSE' failed
  Above error message is shown after 1st game is started.

Since we haven't implemented Pause and Resume functions, and start_game() is meant for only starting a game, I have removed the timer.continue() call.
When we implement those functions, timer.stop() and timer.continue() will be used in them.
Comment 91 Michael Catanzaro 2014-02-28 03:50:59 UTC
Both patches are good, thanks! The commit messages need a bit of work. For the hash map patch, the explanation that you posted in comment 89 ought to go into the body of the commit message.  For both patches, the commit messages need to be wrapped at 72 characters.

You probably want to create the patch using 'git format-patch' [1] and follow the commit message guidelines at [2]. Let me know if you need help with this.

Lastly, it'd be helpful to provide a third patch that removes those two entries from the TODO file. (It's nicer to keep changes to TODO separate from changes to the code.)

[1] https://wiki.gnome.org/Git/Developers#Contributing_patches (not the whole page, just that section is enough)
[2] https://wiki.gnome.org/Git/CommitMessages (don't worry about "Linking to bugs", I'll add the link when I push it)
Comment 92 Parin Porecha 2014-02-28 13:37:10 UTC
Created attachment 270563 [details] [review]
Updated patch to fix Sudoku crash on starting Hard/Very Hard games

(In reply to comment #91)

Thanks for the suggestions !

I am attaching the three updated patches with the changes in commit messages.
I hope the messages aren't too long (thought it was better to justify the changes)
If you think they're long, I'll keep them brief from next time.

> You probably want to create the patch using 'git format-patch' [1] and follow
> the commit message guidelines at [2]. Let me know if you need help with this.

I'll keep that in mind from now on :-)
Comment 93 Parin Porecha 2014-02-28 13:38:17 UTC
Created attachment 270564 [details] [review]
Updated patch to fix GLib critical error related to Sudoku Timer
Comment 94 Parin Porecha 2014-02-28 13:40:41 UTC
Created attachment 270565 [details] [review]
Patch to remove the bug entries in TODO for which I've attached above patches
Comment 95 Michael Catanzaro 2014-02-28 14:44:03 UTC
These all look good.
Comment 96 Michael Catanzaro 2014-02-28 14:44:40 UTC
Er, didn't mean to close this, whoops....
Comment 97 Parin Porecha 2014-05-05 16:12:27 UTC
Created attachment 275897 [details] [review]
Add ability to save a game

As a part of vala port completion, I have implemented the save game feature.

I've mentioned the features of SudokuSaver class in the commit, here are some details -
- Games will be saved in JSON files (like this - http://fpaste.org/99252/) to provide readability.
Cell values are stored explicitly, and the ones which do not have any value or notes assigned to them are skipped. (Thanks Mario for the idea !)

- Everytime a game is saved, a backup is created first. When the save file cannot be read, the latest backup is restored. If all the backups are also corrupt or there isn't any save file present, a random easy board is shown (for now).

- I've updated the game chooser window to show the latest save game. In the future, we'll skip this and directly start the game instead.

- When a game is finished, its savefile is moved to finished/ folder and all its backups are deleted. Printing feature has an option to exclude old games which will now be enabled through this.

I have some questions -
- I've hardcoded the directory inside the class :
+            savegame_dir = Environment.get_home_dir () + "/.config/gnome-sudoku/saved";
+            finishgame_dir = Environment.get_home_dir () + "/.config/gnome-sudoku/finished";

Is it okay? I don't know how to store them in gresource.xml . Help is appreciated.

- If we won't be opening previous saves anytime, why even keep them ?
Right now everytime Sudoku opens, timestamps of all the saves have to be compared to know which is the latest one. If we can keep just a single save (with backups obviously), things will be easier to maintain.

Some clarifications -
- Right now each cell object in the JSON file also contains a field 'notes'. This will be replaced by the earmarked numbers in the future.
- I've added a method 'to_string' in SudokuBoard class which returns the board values as a string.
Comment 98 Michael Catanzaro 2014-05-06 15:43:10 UTC
(In reply to comment #97)
> Created an attachment (id=275897) [details] [review]
> Add ability to save a game
> 
> As a part of vala port completion, I have implemented the save game feature.

Let me respond to your comments here first, before taking a look at your patch. (My promised review will be delayed a bit since I forgot I'm not set up to compile gnome-sudoku on this computer, sorry.)

> I've mentioned the features of SudokuSaver class in the commit, here are some
> details -
> - Games will be saved in JSON files (like this - http://fpaste.org/99252/) to
> provide readability.
> Cell values are stored explicitly, and the ones which do not have any value or
> notes assigned to them are skipped. (Thanks Mario for the idea !)

It's a little verbose, but I think that is fine.  Saving notes like this is fine too -- it means the same code can support arbitrary notes per cell (like current Sudoku), or the single-digit earmark style notes we've been talking about.

> - Everytime a game is saved, a backup is created first. When the save file
> cannot be read, the latest backup is restored. If all the backups are also
> corrupt or there isn't any save file present, a random easy board is shown (for
> now).

This confuses me. If each game is saved in a separate file, why does a backup need to be created?

Since gnome-sudoku will be managing all the save files itself, we shouldn't need to create backups.  We do have to assume any system call will fail, but if saving a game fails (which will almost never happen) then saving the backup is likely to fail as well.

> - I've updated the game chooser window to show the latest save game. In the
> future, we'll skip this and directly start the game instead.

Great.

> - When a game is finished, its savefile is moved to finished/ folder and all
> its backups are deleted. Printing feature has an option to exclude old games
> which will now be enabled through this.

Great!

It doesn't even necessarily have to be exposed as an option (though I think that's fine), if it's done behind the scenes.

> I have some questions -
> - I've hardcoded the directory inside the class :
> +            savegame_dir = Environment.get_home_dir () +
> "/.config/gnome-sudoku/saved";
> +            finishgame_dir = Environment.get_home_dir () +
> "/.config/gnome-sudoku/finished";
> 
> Is it okay? I don't know how to store them in gresource.xml . Help is
> appreciated.

That's almost right. We don't want to store saved games in a gresource. gresources are actually included in the program's binary (/usr/bin/gnome-sudoku) using some ELF witchery [1], so files that need to be edited don't go there.

Finding Environment.get_home_dir() was good, but we don't want to hardcode .config either, since it's possible the user might have chosen a weird location for his config files.  Try Environment.get_user_config_dir() instead.

> - If we won't be opening previous saves anytime, why even keep them ?
> Right now everytime Sudoku opens, timestamps of all the saves have to be
> compared to know which is the latest one. If we can keep just a single save
> (with backups obviously), things will be easier to maintain.

I think the requirements are:

* Have a way to make sure we don't give the user the same Sudoku twice
* Have a way of restoring exactly one unfinished game the user had been playing

So I think a single save is fine.

> Some clarifications -
> - Right now each cell object in the JSON file also contains a field 'notes'.
> This will be replaced by the earmarked numbers in the future.
> - I've added a method 'to_string' in SudokuBoard class which returns the board
> values as a string.

Great.

[1] https://en.wikipedia.org/wiki/Executable_and_Linkable_Format
Comment 99 Mario Wenzel 2014-05-06 15:57:26 UTC
> It's a little verbose, but I think that is fine.
The idea was to avoid adding the implicit constraint to the *parent* data structure to be of length 81

> Saving notes like this is fine too
I would rather it was a set/list of ints. Just because it is cleaner.
Comment 100 Parin Porecha 2014-05-06 16:19:09 UTC
(In reply to comment #98) 
> > - Everytime a game is saved, a backup is created first. When the save file
> > cannot be read, the latest backup is restored. If all the backups are also
> > corrupt or there isn't any save file present, a random easy board is shown (for
> > now).
> 
> This confuses me. If each game is saved in a separate file, why does a backup
> need to be created?
> 
> Since gnome-sudoku will be managing all the save files itself, we shouldn't
> need to create backups.  We do have to assume any system call will fail, but if
> saving a game fails (which will almost never happen) then saving the backup is
> likely to fail as well.

Intention was to provide a degree of redundancy.
It's a harmless feature and completely remains hidden from the user. But since we already save the finished games at another place, backups do not seem necessary.
Should I remove it ?

> Try Environment.get_user_config_dir() instead.

> > - If we won't be opening previous saves anytime, why even keep them ?
> > Right now everytime Sudoku opens, timestamps of all the saves have to be
> > compared to know which is the latest one. If we can keep just a single save
> > (with backups obviously), things will be easier to maintain.
> 
> I think the requirements are:
> 
> * Have a way to make sure we don't give the user the same Sudoku twice
> * Have a way of restoring exactly one unfinished game the user had been playing
> 
> So I think a single save is fine.

Great !
I'll make the necessary changes and post the updated patch.
Comment 101 Michael Catanzaro 2014-05-06 16:25:56 UTC
Review of attachment 275897 [details] [review]:

Parin, this code looks pretty good.  I'm leaving the patch status as None because I haven't had a chance to build and run it yet -- will try to do that later today.

::: src/gnome-sudoku.vala
@@ +33,3 @@
     private SudokuStore sudoku_store;
 
+    private SudokuSaver  saver;

extra space

@@ +119,3 @@
         var hard_grid = (Box) builder.get_object ("hard_grid");
         var very_hard_grid = (Box) builder.get_object ("very_hard_grid");
+        var savegame_grid = (Box) builder.get_object ("savegame_grid");

This will be fine for now.  I agree that in the future we'll want to remove this intro screen and skip straight to the saved game (or a new game).

@@ +179,3 @@
+        var savegame = saver.get_latest_savegame ();
+        if (savegame == null)
+            savegame = new SudokuGame (sudoku_store.get_random_easy_board ());

It'd be a good idea to leave a comment to indicate that this is a FIXME.  (It's fine for the sidebranch.)

::: src/sudoku-board.vala
@@ +488,3 @@
+        }
+
+        return board_string;

Looks like an indentation error

::: src/sudoku-saver.vala
@@ +12,3 @@
+
+            savegame_dir = Environment.get_home_dir () + "/.config/gnome-sudoku/saved";
+            finishgame_dir = Environment.get_home_dir () + "/.config/gnome-sudoku/finished";

Besides using Environment.get_user_config_dir(), also consider using Path.build_path() to create the file path.  (This is just for slightly better portability.)

@@ +28,3 @@
+            }
+        }  catch (Error e) {
+            error ("%s", e.message);

I think calling error() raises SIGTRAP, which will crash the game, create a core dump, and trigger distro bug reporting tools like Apport (Ubuntu) and ABRT (Fedora).   That's probably not what we want to do here, since failures in these calls aren't bugs in gnome-sudoku.  I think we should use warning() here instead of error(), and make sure gnome-sudoku will still work even if we can't create saved games. (Same comment applies to the other uses of error() in this file).

@@ +32,3 @@
+    }
+
+    public SudokuGame get_latest_savegame ()

I think the return type here ought to be a nullable SudokuGame?  Vala doesn't currently make this an error, but maybe it will in the future.  (It will catch hundreds if not thousands of bugs when flipped on.)  Same for get_latest_save_filename() and parse_json_to_game().

@@ +40,3 @@
+                return null;
+
+            game = parse_json_to_game (savegame_dir + "/" + filename, ref game);

Another good place to use Path.build_filename().  (A few more of these below.)

@@ +132,3 @@
+            for (var j = 0; j < board.cols; j++)
+            {
+

extra empty line

@@ +191,3 @@
+            reader.read_member ("position");
+            assert (reader.is_array ());
+            assert (reader.count_elements () == 2);

Your use of asserts in this function are good because it makes the code easier to read and will probably stop somebody from breaking something in the future, but bad because the saved game file is untrusted input, and we don't want a processing error to crash gnome-sudoku.  I think you can use return_if_fail(null) instead.

@@ +221,3 @@
+        reader.read_member ("time_elapsed");
+        assert (reader.is_value ());
+        game.board.previous_played_time = (double) reader.get_double_value ();

Oops: you're casting a double to a double!
Comment 102 Michael Catanzaro 2014-05-06 16:29:35 UTC
(In reply to comment #100)
> Intention was to provide a degree of redundancy.
> It's a harmless feature and completely remains hidden from the user. But since
> we already save the finished games at another place, backups do not seem
> necessary.
> Should I remove it ?

I think so.  If we were working on an airplane, I would ask for more backups. For Sudoku, I think the extra code complexity doesn't justify the minimal benefit.
Comment 103 Parin Porecha 2014-05-07 07:40:13 UTC
Created attachment 276047 [details] [review]
Updated patch to add ability for saving a game

Here's the updated patch.

- Corrected indentation and space errors
- Changed error () calls to warning ()
- Changed path string concatenations to Path.build_path ()
- assert () -> return_val_if_fail (null)
- Removed backups
- Removed 'last_modified' field from save data (we don't compare timestamps anymore, and this value can be retrieved from FileInfo anyways).
Comment 104 Parin Porecha 2014-05-08 11:57:25 UTC
Created attachment 276152 [details] [review]
Patch which enables 'markasPlayedToggle' and 'includeOldGamesToggle' options in multiple print dialog

Commit message pasted below

Enabled the options 'markasPlayedToggle' and 'includeOldGamesToggle' in multiple print dialog
    
When 'markasPlayedToggle' is enabled, the games which are printed will be added to the finished games folder.
'includeOldGamesToggle' if disabled, get_assorted_boards () method in SudokuStore will not include a board if it exists in finished/ folder.
    
Miscellaneous -
- Removed unused variable 'file' from SudokuPrinter
- Added a public method 'is_finished ()' to SudokuBoard class which can be used to know if a board has been completed or not.
- variables 'finishgame_dir' and 'savegame_file' have been made static so that classes which don't have access to a SudokuSaver instance can still use them
Comment 105 Michael Catanzaro 2014-05-08 16:38:00 UTC
Review of attachment 276047 [details] [review]:

OK, good (and apologies again for the delay -- I want to be faster at reviewing your changes).

I made one mistake in my last review: the saved games should go under ~/.local/share/gnome-sudoku, not ~/.config/gnome-sudoku.  Use Environment.get_user_data_dir() rather than Environment.get_user_config_dir().  You can push this after making that change.

In the future, please create your patches using either git format-patch [1] or git-bz [2], following the commit message guidelines [3]

[1] https://wiki.gnome.org/Git/Developers#Contributing_patches
[2] http://fishsoup.net/software/git-bz/
[3] https://wiki.gnome.org/Git/CommitMessages
Comment 106 Michael Catanzaro 2014-05-08 16:52:44 UTC
Review of attachment 276152 [details] [review]:

Looks fine.

One issue with the Print Multiple dialog that I didn't notice before and which was not introduced by this patch: the access keys (mnemonics) that appear when you hold down Alt are not unique. "Print" and "Puzzles per page" both use P, and "Medium" and "Mark games as played once you've played them" both use M.  The access keys have to be unique, but not necessarily the first letter of the label.

[1] https://wiki.gnome.org/Design/HIG/Typography
Comment 107 Parin Porecha 2014-05-09 09:54:40 UTC
Created attachment 276226 [details] [review]
Updated SudokuSaver patch

Changed config dir to user data dir
Comment 108 Michael Catanzaro 2014-05-13 15:56:34 UTC
Comment on attachment 276152 [details] [review]
Patch which enables 'markasPlayedToggle' and 'includeOldGamesToggle' options in multiple print dialog

Attachment 276152 [details] pushed as 8c40c27 - Patch which enables 'markasPlayedToggle' and 'includeOldGamesToggle' options in multiple print dialog
Comment 109 Parin Porecha 2014-05-15 16:29:26 UTC
Created attachment 276612 [details] [review]
Patch to add ability of generating sudokus

Completed SudokuGenerator class port.

Features added through this patch -
- When Sudoku starts, the puzzle data files will be copied to user data folder (if they don't already exist there), and SudokuStore will use them instead.
- 10 Sudokus of each difficulty category will be generated per 2 minutes which will then be appended to the data files.
- On completing a puzzle, a puzzle of same difficulty will be generated and added.

However, please remain cautious while testing this patch. The SudokuSolver's method .has_unique_solution() gets stuck frequently (it happened at least once while generating the 40 sudokus at a time) during which CPU utilization can go upto 25% and Memory usage will go on increasing until it gets out of the deadlock (or whatever's happening inside) or manually the process is killed.

The no. of sudokus to be generated a time and the timer interval has been hard-coded for now because the patch is not fit to be merged. And since the Solver and Generator legacy code is over-complicated and takes too much time (because of the deadlocks), it'll be replaced in future with a simpler and faster algo.
Comment 110 Parin Porecha 2014-05-16 04:59:31 UTC
Created attachment 276651 [details] [review]
Updated patch for Sudoku Generator

Removed all the debug statements, and '--target-glib=2.32' from src/Makefile.am as we're no longer using threads.
Comment 111 Michael Catanzaro 2014-05-16 19:44:00 UTC
Review of attachment 276651 [details] [review]:

So the current challenge is to fix the infinite loop....

::: src/sudoku-generator.vala
@@ +14,3 @@
         generate_start_board(-1, -1, -1, -1, ref solution);
+
+        Timeout.add_full (Priority.LOW, 120000, regularly_generate_boards);

A few things:

*Can you change this to Timeout.add_seconds_full?  This reduces wakeups, and therefore power consumption.
* regularly_generate_boards() should only generate new boards if it has fewer boards than some minimum. It doesn't need to keep building up its database of puzzles forever, which as far as I can tell is what it does now.
* With that change, we can call it more frequently, like every 10s or 30s or so. This way, we're less likely to run out of puzzles if I print 100 (currently the maximum) sudokus using Print Multiple, then do so again right away.

@@ +24,3 @@
             for (int repeat = 0; repeat < 50; repeat++)
             {
+                stdout.printf ("repeat %d\n", repeat);

Did you miss these debug statements (several more throughout this file), or are you leaving them to help debug the infinite loop?

If we leave them, use debug() to print them, instead of writing unconditionally to stdout. This will ensure they're not printed normally, but will be when G_MESSAGES_DEBUG=all is defined.

I have the following line in my jhbuildrc:

os.environ['G_MESSAGES_DEBUG'] = "all"

@@ +32,1 @@
                 while (true) {

This seems fragile. I think SudokuGenerator.make_unique_puzzle() should be modified to never return null.

@@ +60,3 @@
+        saver.add_boards_to_data_files (boards);
+
+        return true;

My first though was "looks like this function ought to be void," but I see that the return value controls whether this function remains attached to the main loop. To make this more clear, you can either:

a) Add a comment:

/* Remain in main loop */
return true;

b) Depend on Vala 0.25 (as-yet unreleased) and use this constant instead:

return Source.CONTINUE;

(b) is obviously better if you're willing to bump the Vala dependency. (Makes sense if you're using jhbuild, especially since this is a sidebranch and unlikely to bother anyone.)

::: src/sudoku-saver.vala
@@ +44,3 @@
+
+            {
+                // Copy data files

Why this extra scope? I would understand if you were limiting the scope of a variable declaration, but you're not doing that.

@@ +246,3 @@
+    {
+        try {
+            var file = File.new_for_uri ("resource:///org/gnome/gnome-sudoku/puzzles/" + level);

Trivial: the file loaded into the resource contains the default puzzles, correct? This was a bit confusing to me at first, so a comment might be appropriate, or simply a more descriptive method name like create_default_puzzle_file() (but then you'd need to move the check if local_data_file.query_exists() outside this function, which might be more appropriate with the current function name anyway, since this function doesn't always copy the data file... perhaps maybe_copy_data_file() would be more appropriate).
 
Also, a trivial optimization: this variable looks like it should be declared in the if statement below (since that is the only place it is used, and avoids the extra open when local_data_file exists, which should be usually).
Comment 112 Parin Porecha 2014-05-17 05:28:11 UTC
(In reply to comment #111)
> Did you miss these debug statements (several more throughout this file), or are
> you leaving them to help debug the infinite loop?
> 
> If we leave them, use debug() to print them, instead of writing unconditionally
> to stdout. This will ensure they're not printed normally, but will be when
> G_MESSAGES_DEBUG=all is defined.

These are a part of gen_stats() . Supposed to show stats while creating unique puzzles. Since this function hasn't been called anywhere, and eventually the whole thing's going to be replaced, I didn't remove it.
Comment 113 Parin Porecha 2014-05-24 07:42:03 UTC
Created attachment 277100 [details] [review]
Move 'Show Possible Numbers' feature to a CLI option

To enable the feature, use '--show-possible-values' or '-s'
Comment 114 Parin Porecha 2014-05-24 08:36:02 UTC
Created attachment 277101 [details] [review]
Show 'Clear' button in popover number-picker for a filled cell

Since the NumberPicker is initialized at the time of SudokuCellView initialization, the 'Clear' button did not get shown when user fills a value in a cell. This patch is a fix for this bug.
Comment 115 Michael Catanzaro 2014-05-24 20:00:11 UTC
Comment on attachment 277101 [details] [review]
Show 'Clear' button in popover number-picker for a filled cell

Both look good.

Have you heard back from the Accounts team about your git access yet?  (Mario and I were asked to vouch for you right aftetr you applied.) If so, you should be able to push these yourself.
Comment 116 Parin Porecha 2014-05-25 06:59:52 UTC
Thanks for vouching, my request was resolved.
Two patches in https://bugzilla.gnome.org/show_bug.cgi?id=710624#c26 haven't been reviewed.
Third patch 'Add undo, redo' was reviewed, but its status hasn't changed.
For accels, I'll post a separate patch assigning for all the actions that need them - undo, redo, print, restart etc. (we'll decide which ones need a shortcut, which ones don't).
Once you review them, I'll push all the five.
Comment 117 Parin Porecha 2014-05-27 11:41:23 UTC
Created attachment 277283 [details] [review]
Use Popover for number picker and earmarking

Some questions :
- Earlier we had 2 notes for each cell, so 25% at the top was assigned to top notes and 25% at bottom to bottom notes. Now, since we are using single earmarking should we change the ratio of click location ?

- We're using popovers for both number picker and earmarking. For empty cells, there's no way to know without clicking inside, whether the popover which has been opened is for assigning a value to the cell or for setting earmarks. We need to set some difference between the two. Something like contrasting backgrounds. Michael, Allan, suggestions ?
Comment 118 Parin Porecha 2014-05-27 11:48:57 UTC
Regarding committing the previous patches, sorry, I'd lost my ssh key so I've asked accounts team to replace it. I'm hoping that the issue will be resolved in a day or two.
Comment 119 Michael Catanzaro 2014-05-27 14:05:00 UTC
Review of attachment 277283 [details] [review]:

Looks good, for now.
Comment 120 Michael Catanzaro 2014-05-27 14:19:18 UTC
I'm not very good at design work, but until we hear otherwise, let's do this:

* The location of your click within the cell should not matter. Ctrl+Click or right click brings up the earmark popover. Left click brings up the normal popover.  This is probably not sufficiently discoverable, but it will do until Allan has time to take a look.
* Earmarks should display across the top, not in number pad format throughout the cell. I thought the number pad format would work fine, but it fails badly if the user selects a number for that cell, which we want to allow.

Regardless - things are shaping up pretty well, good job. I think we'll be ready to merge vala-port into master soon, and possibly even do an unstable release with GNOME 3.13.3.
Comment 121 Michael Catanzaro 2014-05-27 14:20:28 UTC
Comment on attachment 277283 [details] [review]
Use Popover for number picker and earmarking

Attachment 277283 [details] pushed as 4baa057 - Use Popover for number picker and earmarking
Comment 122 Parin Porecha 2014-05-28 09:56:21 UTC
Created attachment 277372 [details] [review]
Show earmark picker on right-click and Ctrl-Click and show earmarked numbers inline across the top

(In reply to comment #120)
> * Earmarks should display across the top, not in number pad format throughout
> the cell. I thought the number pad format would work fine, but it fails badly
> if the user selects a number for that cell, which we want to allow.

Notes had a bug - there was no limit on the no. of characters, so when overflow occurred, only those chars that could fit in the cell width were displayed.
Showing earmarks inline is causing the same problem.
Trying to show all 9 numbers inline requires their size to be very small and causes aliasing - http://i.imgur.com/nzFVDRw.png
But since chances that a user will select all the 9 are very very less, I have set the size ratio such that at max 7 numbers can be shown - http://i.imgur.com/jQRn6u6.png
If you feel this does not look good, I'll update the patch.

> Regardless - things are shaping up pretty well, good job. I think we'll be
> ready to merge vala-port into master soon, and possibly even do an unstable
> release with GNOME 3.13.3.

Great to hear that !
Comment 123 Michael Catanzaro 2014-05-29 04:33:17 UTC
Review of attachment 277372 [details] [review]:

Cool.

I'd recommend cutting it off at even fewer digits so that you can get the earmarks a bit bigger -- maybe 5 -- and modifying the number picker such that at most that many digits can be selected at a time.

::: src/sudoku-view.vala
@@ +204,3 @@
         }
 
+        if (event.button == 1)

Looks like this is indeed the only solution, but can you add a comment to make it clear that 1 is primary click, 2 is secondary click?
Comment 124 Parin Porecha 2014-05-29 06:53:41 UTC
Created attachment 277434 [details] [review]
Show earmark picker on right-click and Ctrl-Click and show earmarked numbers inline across the top

Patch updated to show at max 5 numbers.
When the limit is reached, rest of the number buttons become insensitive.
Comment 125 Parin Porecha 2014-05-30 06:47:35 UTC
(In reply to https://bugzilla.gnome.org/show_bug.cgi?id=710624#c16)
> My suggestion is to place New Game, Start Over, and Change Difficulty buttons
> starting from the bottom-right and working up, similar to what we're doing in
> Mines 13.1. I'll ping Allan to see if he has a better plan.

'Start Over' will reset the board, but what should be done with 'New Game' and 'Change Difficulty' ?
Maybe we can bring back the game chooser screen or use a dialog with a combobox.
And, do we really need both the new game and change difficulty buttons ?
The only difference I can think of between them is to not show current level in change difficulty screen/dialog. The overlap is fairly big, and if the user does not want to play the same level again, he can simply select another in New Game screen. To me, having 2 buttons out of which one's functionality is a subset of the other does not seem so useful.

But, if you're thinking about showing a chooser screen only for 'New Game' and directly starting a random board on 'Change Difficulty', that would be a different case and I agree with your suggestion.
Comment 126 Mario Wenzel 2014-05-30 08:46:19 UTC
>Maybe we can bring back the game chooser screen or use a dialog with a
combobox.

We want to keep the dialogues to a minimum.

If you are unsure about the new-game/difficulty thing you could try and combine them for starters.
New Game left click starts a new game with same difficulty. Right click gives you a context menu with the difficulties. Pressing one would start a game with that specific difficulty.
Or something similar.

But we couldn't decide on how to make that visible to the user.

But I'd agree with Michael in general. We decided on three buttons, until we have a better idea. Start Over -> Same game, all cleared; New Game -> New Game, same difficulty; -> Choose Difficulty -> New Game, New Difficulty
It's a natural progression that way
Comment 127 Michael Catanzaro 2014-05-30 13:30:44 UTC
(In reply to comment #126)
> New Game left click starts a new game with same difficulty. Right click gives
> you a context menu with the difficulties. Pressing one would start a game with
> that specific difficulty.
> Or something similar.

Right/left click on a text button should not make any difference

(In reply to comment #125)
> And, do we really need both the new game and change difficulty buttons ?
> The only difference I can think of between them is to not show current level in
> change difficulty screen/dialog. The overlap is fairly big, and if the user
> does not want to play the same level again, he can simply select another in New
> Game screen. To me, having 2 buttons out of which one's functionality is a
> subset of the other does not seem so useful.

I think I was wrong: we don't need two separate buttons. We do want both buttons in games that are short because with short games, it is annoying to go through a new game screen each time if you want to stick to the same difficulty (the typical case).  For Sudoku, it's OK to show the new game screen each time because the puzzles take a long time to solve, and one extra click to start a new game is not a problem at all.

We do want to bring back the new game screen, but... I trust you can make it look a bit better than the implementation you removed. :)  I'm sorry we don't have a design for this.
Comment 128 Parin Porecha 2014-05-31 13:56:38 UTC
I tried my hand at a slightly modified and compact version of the older new game screen - http://i.imgur.com/ipkIhGb.png

This will improve with adding some margin between the two rows, and changing the view (cell spacing, border width etc.) to suit the less space.
About the Undo, Redo buttons, they'll be replaced by a 'Back' button which will cancel the operation and return back to the board being played. HeaderBar title will also change. But, these are minor things.

We need to decide how the chooser should look. Comments welcome on the new implementation.
If this looks cramped and aliased, another way would be to use 4 buttons just like Mines - http://i.imgur.com/OEypgVf.png
Comment 129 Michael Catanzaro 2014-05-31 14:48:30 UTC
Review of attachment 277434 [details] [review]:

I guess I missed this, sorry!

::: src/number-picker.vala
@@ +12,3 @@
     private Button clear_button;
 
+    public const int EARMARKS_MAX_ALLOWED = 5;

I think this should be private, and also static (since different NumberPickers do not have a different number of earmarks allowed).
Comment 130 Michael Catanzaro 2014-05-31 15:06:03 UTC
FYI:  I've merged vala-port into master, since you're on track to finish well before 3.14.  From now on, you can push your patches directly into master. If something goes horribly wrong and we need to delay the first Vala release, gnome-3-14 can be branched off of gnome-3-12.

Also, let's start making new bugs for individual issues, to make it easier to follow the discussion.  As far as I can tell, the remaining issues tracked in this bug are:

* UI changes
* Sudoku generator is somewhat broken

(In reply to comment #128)
> I tried my hand at a slightly modified and compact version of the older new
> game screen - http://i.imgur.com/ipkIhGb.png
>
> This will improve with adding some margin between the two rows, and changing
> the view (cell spacing, border width etc.) to suit the less space.
> About the Undo, Redo buttons, they'll be replaced by a 'Back' button which will
> cancel the operation and return back to the board being played. HeaderBar title
> will also change. But, these are minor things.
>
> We need to decide how the chooser should look. Comments welcome on the new
> implementation.
> If this looks cramped and aliased, another way would be to use 4 buttons just
> like Mines - http://i.imgur.com/OEypgVf.png

OK.  This will be much better than the original screen.

I think eventually we might want to show just buttons instead, without the puzzles, but it might be hard to make that look good without a design.
Comment 131 Parin Porecha 2014-06-01 12:33:05 UTC
Created attachment 277676 [details] [review]
Replace static number picker with New Game & Restart buttons

'New Game' and 'Reset' options have been removed from the Appmenu.
Comment 132 Michael Catanzaro 2014-06-01 17:19:05 UTC
Review of attachment 277676 [details] [review]:

Good work.  I think we should try to make this look a bit better; as you've noticed, finding appropriate icons for this is not easy.

My suggestion: use two text-only buttons, packed from the end (so that they appear at the bottom of the screen).  The buttons should read "Clear Board" and "New Puzzle."  Both should not do anything until the user passes through a confirmation dialog.  Clear Board is Reset Game; New Puzzle takes the user to your new game screen.
Comment 133 Parin Porecha 2014-06-02 06:36:03 UTC
Created attachment 277708 [details] [review]
Replace static number picker with New Game & Restart buttons

Patch updated.

I haven't given much thought to the message in 'New puzzle' confirmation dialog because anyways it'll be replaced by the new game screen.
Comment 134 Michael Catanzaro 2014-06-02 13:03:07 UTC
(In reply to comment #133)
> I haven't given much thought to the message in 'New puzzle' confirmation dialog
> because anyways it'll be replaced by the new game screen.

That's right: I forgot you were going to add a Cancel button to the new game screen.
Comment 135 Parin Porecha 2014-06-02 16:19:01 UTC
Created attachment 277738 [details] [review]
Show game chooser screen on clicking 'New Game' button

I've set the preview sizes such that the game window doesn't resize when the screen switching happens.

In the game chooser screen, no header bar subtitle will be shown and (Undo, Redo) buttons will be replaced by 'Back'.
Comment 136 Michael Catanzaro 2014-06-03 01:51:02 UTC
Created attachment 277773 [details] [review]
Show earmark picker on right-click and Ctrl-Click

and show earmarked numbers inline across the top
Comment 137 Michael Catanzaro 2014-06-03 02:01:56 UTC
Review of attachment 277738 [details] [review]:

Parin, this is looking very good.

Still, enough small mistakes that I will feel like a useful mentor again. :D  After fixing these, you can push without waiting for another review.

::: data/gnome-sudoku.ui
@@ +57,3 @@
+                        <property name="halign">center</property>
+                        <property name="valign">center</property>
+                        <property name="label" translatable="yes">Back</property>

Can you give this a mnemonic (access key)?  You do this by placing an underscore before the character that you want to be the access key.  B works well here.  You'll usually want to use the first character, but you never want two different buttons or labels to have the same access key.  Check this by holding down Alt to display all the access keys in the current window.  (That's also how you activate them: Alt+B, for example.)

@@ +59,3 @@
+                        <property name="label" translatable="yes">Back</property>
+                        <property name="tooltip-text" translatable="yes">Go back to the current game</property>
+                        <property name="use_underline">True</property>

When you add a mnemonic, you need to set use_underline=True, otherwise you'll just have a useless underline show up in your button label (which you'll notice pretty quickly - it's obvious). The Gtk.Button.with_mnemonic() constructor handles this for you. (Note that since you already have this here, there's nothing more to do.)

::: src/gnome-sudoku.vala
@@ +167,3 @@
+        var medium_grid = (Box) builder.get_object ("medium_grid");
+        var hard_grid = (Box) builder.get_object ("hard_grid");
+        var very_hard_grid = (Box) builder.get_object ("very_hard_grid");

OK, so the big problem here is that New Game button is no longer actually capable of creating a new game, just of switching between these four preexisting ones.  Can you fix this?
Comment 138 Michael Catanzaro 2014-06-03 02:02:05 UTC
Review of attachment 277708 [details] [review]:

Looks good. I have a few suggestions.  After fixing these, you can push without waiting for another review.

After using Clear Board, you can still Undo and Redo your previous moves, but nothing happens. This should clear the undo stack.  (Better would be to make it possible to undo the Clear Board action, in which case you wouldn't need the confirmation dialog, but that might be harder.)

::: src/gnome-sudoku.vala
@@ +122,3 @@
+
+        var new_button = new Gtk.Button ();
+        var new_label = new Gtk.Label ("New Puzzle");

Use the Gtk.Label.with_mnemonic() constructor to give this button a mnemonic: Gtk.Label.with_mnemonic ("_New Puzzle")

@@ +128,3 @@
+        new_button.halign = Gtk.Align.CENTER;
+        new_button.action_name = "app.new-game";
+        new_button.tooltip_text = _("Start a new puzzle with random difficulty");

tooltip should be updated (simply "Start a new puzzle")

@@ +133,3 @@
+
+        var restart_button = new Gtk.Button ();
+        var restart_label = new Gtk.Label ("Clear Board");

Also needs a mnemonic

@@ +246,1 @@
     private void new_game_cb ()

I think this function is unused. :)

It's better to provide an Undo button when it works well, or a Back button in this case, than to disrupt the user with a confirmation dialog, so I think the current implementation is fine and you can just remove this function.

@@ +262,3 @@
     private void reset_cb ()
     {
+        var dialog = new MessageDialog(window, DialogFlags.DESTROY_WITH_PARENT, MessageType.INFO, ButtonsType.OK_CANCEL, "Reset the board to its original state?");

Missing space! :)

Instead of using DialogFlags.DESTROY_WITH_PARENT, how about DialogFlags.MODAL? This prevents the user from moving the dialog out of the way and continuing to play.

Also, MessageType.QUESTION rather than MessageType.INFO.  (This used to affect the image that gets shown in the dialog.  Now that there is no longer an image, I'm not sure if it makes any difference, but might as well set it properly.)

All of these comments would also apply to the dialog in new_game_cb()
Comment 139 Michael Catanzaro 2014-06-03 02:10:43 UTC
Comment on attachment 277773 [details] [review]
Show earmark picker on right-click and Ctrl-Click

Attachment 277773 [details] pushed as 15c33fa - Show earmark picker on right-click and Ctrl-Click
Comment 140 Parin Porecha 2014-06-03 16:04:03 UTC
Comment on attachment 277708 [details] [review]
Replace static number picker with New Game & Restart buttons

Attachment 277708 [details] pushed as 7cd0dbd - Replace static number picker with New Game & Restart buttons
Comment 141 Parin Porecha 2014-06-03 16:07:43 UTC
Comment on attachment 277738 [details] [review]
Show game chooser screen on clicking 'New Game' button

> OK, so the big problem here is that New Game button is no longer actually
> capable of creating a new game, just of switching between these four
> preexisting ones.  Can you fix this?

Sorry for overlooking this major bug.
Fixed and pushed as 384f4f3 - Show game chooser screen on clicking 'New Game' button
Comment 142 Michael Catanzaro 2014-06-04 14:18:12 UTC
Comment on attachment 277434 [details] [review]
Show earmark picker on right-click and Ctrl-Click and show earmarked numbers inline across the top

I committed this one a few days ago
Comment 143 Michael Catanzaro 2014-06-04 14:19:34 UTC
(In reply to comment #141)
> Sorry for overlooking this major bug.

It happens. :)

Check out Allan's new wireframes - you don't have to resolve this at all if you update to that design.
Comment 144 Michael Catanzaro 2014-06-04 14:21:31 UTC
(In reply to comment #143)
> (In reply to comment #141)
> > Sorry for overlooking this major bug.
> 
> It happens. :)
> 
> Check out Allan's new wireframes - you don't have to resolve this at all if you
> update to that design.

Ah, well I see you already did: great.

The only issue remaining in this bug is SudokuGenerator.  I think it's time to close this, and track Generator in a separate bug.  Good work!