This uses http://fc-solve.shlomifish.org/ and prevents the looping in the existing solvers.
Details
Details
Test that the solvers are working.
Diff Detail
Diff Detail
- Repository
- R410 KPatience
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Do you have a game number we can use to compare how this is better than the existing code?
Also i think you need to merge master changes.
| CMakeLists.txt | ||
|---|---|---|
| 8 | Do you know how that library is packaged by "the major distros", whatever "the major distros" means :D | |
| patsolve/abstract_fc_solve_solver.cpp | ||
| 3 | Is the code in all these new files really by Stephan? Or are you gifting him your code? | |
| patsolve/freecellsolver.cpp | ||
| 41 | Just remove the code if it's not needed. | |
Comment Actions
Considering that currently no one (including me, the de-facto maintainer) really knows what the solver is doing exactly and how the numbers for the search in it were derived, I appreciate outsourcing the logic to a library. However, like @aacid said, it would be nice to have either some numbers how the solver compares.
| CMakeLists.txt | ||
|---|---|---|
| 8 | As far as I can see, Debian and Fedora have this library, but Arch and openSuse do not. @shlomif: Do you think it would make sense to have the library as an optional dependency for now? Or do you think that it is so much better that we should push distros to package it? | |
| freecell.cpp | ||
| 136 | Ideally, new code shouldn't have commented out debug messages. But when it does (because you might want to work on that part of the code in the future), it should at least use qDebug or QLogginCategory, so that we do not introduce a hidden dependency on a deprecated framework. | |
| patsolve/abstract_fc_solve_solver.cpp | ||
| 22 | You're pulling in iostream here, which has the disadvantage of being slow to compile. As far as I can see, this is only needed for debug printing. In that case, can you also wrap the includes in a #if 0? Or even better, guard everything with something like #ifdef PRINT_DEBUG, to make clear what the commented out code is for. | |
| patsolve/abstract_fc_solve_solver.h | ||
| 24 | You'll need to change this against master, as I did some refactoring of the Solver class. If the fact that Solver is now a template confuses you, you might want to take a look at the discussion in https://phabricator.kde.org/D11815. Or just ask me, I won't bite ;-) | |
| patsolve/freecellsolver.cpp | ||
| 238 | Not being familiar with the Freecell Solver library: How were those parameters chosen? | |
Comment Actions
Hi all!
Thanks for the comments. I began applying them on my branch at https://github.com/shlomif/kpat/tree/fc-solve-integration . You can try pressing "D" on Simple Simon deal no. 24 in both branches. Regarding enforcing the use of the libfreecell-solver library, I am biased because I originated it, but I can try working on a compile-time fallback to the built-in code.
Comment Actions
Update to the master branch and apply commentary from the reviewers - no iostream, no kDebug, precanned solving themes, etc.
Comment Actions
Sorry, I technically reviewed it already, but forgot to hit submit :-(. I'm happy with the current state of the code (sans that one copyright line), and the solver does a better job than the current one.
So the only question is whether to add the library as a hard dependency. Personally, I'm fine with that, and unconditionally depending on it makes the kpat code easier to understand and maintain. The only question is whether this is an issue for the distributions. It shouldn't be an issue for Debian based ones, as they have it in their repos, and there exists a RPM which is already in some distros repos, but not in all. For instance, not in Fedora (contrary to what I initially thought). @aacid : Do you know anyone from the distro side who could chime in if adding the library would pose an issue?
| patsolve/abstract_fc_solve_solver.h | ||
|---|---|---|
| 3 | That copyright line is probably wrong, isn't it? | |
Comment Actions
@fabiank : from what I seem to recall, that new file was derived from an existing one that carried that copyright. I on my part disclaim any ownership for my modifications to this file.
Comment Actions
https://mail.kde.org/mailman/listinfo/kde-distro-packagers is the place where you can all the distro people about it, would you do it?
Comment Actions
Oh well, time to get this merged. I didn't get *too* strong objection from the mailing list, so I intend to merge this as is. Thank you very much @shlomif for your patience and your code contribution.
Comment Actions
Problem is that now continuous integration fails because the lib is not available.
Can you please open a ticket at http://sysadmin.kde.org/tickets to get it installed?
https://build.kde.org/job/Applications%20kpat%20kf5-qt5%20SUSEQt5.9/19/console