D12415: Use Freecell Solver for FreeCell and Simple Simon

[Fabian Kosmale]

fabiank added a comment.


  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.

INLINE COMMENTS

> CMakeLists.txt:8
> +include(FindPkgConfig)
> +pkg_check_modules(FC_SOLVE REQUIRED libfreecell-solver)
>  find_package(ECM ${KF5_MIN_VERSION} REQUIRED CONFIG)

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
> +        QString a = QString::fromLatin1("Freecells: %1\n");
> +        // kDebug(11111) << "a is {{{{" << a << "}}}}" << endl;
> +        output += a.arg(tmp);

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.

> abstract_fc_solve_solver.cpp:21
> +
> +#include <iostream>
> +

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.

> abstract_fc_solve_solver.h:23
> +
> +class FcSolveSolver : public Solver
> +{

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 ;-)

> freecellsolver.cpp:238
>  
> -int FreecellSolver::get_possible_moves(int *a, int *numout)
> +static const char * freecell_solver_cmd_line_args[CMD_LINE_ARGS_NUM] =
>  {

Not being familiar with the Freecell Solver library: How were those parameters chosen?

REPOSITORY
  R410 KPatience

REVISION DETAIL
  https://phabricator.kde.org/D12415

To: shlomif, #kde_games, fabiank
Cc: aacid, #kde_games
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20180423/f178c2e6/attachment.html>