<table><tr><td style="">fabiank added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D12415">View Revision</a></tr></table><br /><div><div><p>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 <a href="https://phabricator.kde.org/p/aacid/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@aacid</a> said, it would be nice to have either some numbers how the solver compares.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12415#inline-63415">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:8</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">include(FindPkgConfig)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">pkg_check_modules(FC_SOLVE REQUIRED libfreecell-solver)
</div><div style="padding: 0 8px; margin: 0 4px; ">find_package(ECM ${KF5_MIN_VERSION} REQUIRED CONFIG)
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">As far as I can see, Debian and Fedora have this library, but Arch and openSuse do not. <a href="https://phabricator.kde.org/p/shlomif/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@shlomif</a>: 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?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12415#inline-63417">View Inline</a><span style="color: #4b4d51; font-weight: bold;">freecell.cpp:136</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">a</span> <span style="color: #aa2211">=</span> <span class="n">QString</span><span style="color: #aa2211">::</span><span class="n">fromLatin1</span><span class="p">(</span><span style="color: #766510">"Freecells: %1</span><span style="color: #bb6622">\n</span><span style="color: #766510">"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// kDebug(11111) << "a is {{{{" << a << "}}}}" << endl;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">output</span> <span style="color: #aa2211">+=</span> <span class="n">a</span><span class="p">.</span><span class="n">arg</span><span class="p">(</span><span class="n">tmp</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12415#inline-63419">View Inline</a><span style="color: #4b4d51; font-weight: bold;">abstract_fc_solve_solver.cpp:21</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><iostream></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12415#inline-63418">View Inline</a><span style="color: #4b4d51; font-weight: bold;">abstract_fc_solve_solver.h:23</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">class</span> <span style="color: #a0a000">FcSolveSolver</span> <span class="p">:</span> <span class="n">public</span> <span class="n">Solver</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 <a href="https://phabricator.kde.org/D11815" class="remarkup-link" target="_blank" rel="noreferrer">https://phabricator.kde.org/D11815</a>. Or just ask me, I won't bite ;-)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D12415#inline-63420">View Inline</a><span style="color: #4b4d51; font-weight: bold;">freecellsolver.cpp:238</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"> </span><span class="n"><span class="bright">F</span>reecell<span class="bright">S</span>olver<span class="bright"></span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">get_possible_moves</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"></span><span class="n"><span class="bright">a</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"></span><span class="n"><span class="bright">numout</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"></span><span style="color: #aa4000"><span class="bright">static</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">char</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span class="n"><span class="bright">f</span>reecell<span class="bright">_s</span>olver<span class="bright">_cmd_line_args</span></span><span class="bright"></span><span class="p"><span class="bright">[</span></span><span class="bright"></span><span class="n"><span class="bright">CMD_LINE_ARGS_NUM</span></span><span class="bright"></span><span class="p"><span class="bright">]</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not being familiar with the Freecell Solver library: How were those parameters chosen?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R410 KPatience</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12415">https://phabricator.kde.org/D12415</a></div></div><br /><div><strong>To: </strong>shlomif, KDE Games, fabiank<br /><strong>Cc: </strong>aacid, KDE Games<br /></div>