<table><tr><td style="">jjazeix 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/D10950" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Thank you for your contribution, it's a really nice patch (most of the remarks are minor ones).</p>

<p>c++ files are compiled twice (one for the lib, one for the executable). I remembered I had issues (crashes on android) when I tried to link the executable to the library (<a href="https://github.com/gcompris/GCompris-qt/blob/networkClient/src/core/CMakeLists.txt#L66" class="remarkup-link" target="_blank" rel="noreferrer">https://github.com/gcompris/GCompris-qt/blob/networkClient/src/core/CMakeLists.txt#L66</a> on this branch).</p>

<p>Do you think it can be applied to activities too (as the code is mostly qml and js, it may be more difficult to unit tests them, making automatic tests more viable and interesting to avoid regressions)?</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/D10950#inline-52641" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:462</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);"># Tell CMake to run moc when necessary:
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">set(CMAKE_AUTOMOC ON)
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">if you use automoc, can you remove the qt5_wrap_cpp in the src/core folder and check if it still works?<br />
You may need to move this line higher on the file to also apply to src subdirectory</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/D10950#inline-52663" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:466</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);"># to always look for includes there:
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">set(CMAKE_INCLUDE_CURRENT_DIR ON)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">add_subdirectory(tests)
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">same here, we used include_directories() in the src/core, it may not be needed anymore with this.</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/D10950#inline-52667" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:80</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);">add_library(GCompris SHARED ${gcompris_SRCS} ${gcompris_MOC} ${gcompris_RES})
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">target_link_libraries(GCompris
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I would name it "gcompris_core" instead of "GCompris" and I would not add "gcompris_RES" to it as they are the resource files and so not linked to the library itself but the executable</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/D10950#inline-52671" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:3</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_directories(
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    ${CMAKE_SOURCE_DIR}/src/core
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">it would be preferable to not include the folders and use #include "src/core/ActivityInfo.h" instead (so we know where to look for when we look for the file).</p>

<p style="padding: 0; margin: 8px;">Also, you probably don't need the resource include</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/D10950#inline-52685" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:14</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);">add_executable(GComprisActivityInfoTest activityinfotest.cpp)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">target_link_libraries(GComprisActivityInfoTest ${CORE_TEST_LIBRARIES})
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">it would be better to prefix it with Core instead of GCompris. Basically, all the tests will be for GCompris so no need to add it but we may have different modules (activities, core...).</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/D10950#inline-52655" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">activityinfotest.cpp:1</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"><QtTest></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QObject></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">missing licence header</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/D10950#inline-52669" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">activityinfotest.cpp:2</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"><QtTest></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QObject></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">filename should be ActivityInfoTest.cpp</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/D10950#inline-52664" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">activityinfotest.cpp:6</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">Q_DECLARE_METATYPE</span><span class="p">(</span><span style="color: #aa4000">unsigned</span> <span style="color: #aa4000">int</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why do you need this?</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/D10950#inline-52634" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">activityinfotest.cpp:41</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="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">  <span class="n">ActivityInfo</span><span style="color: #aa2211">*</span> <span class="n">activityinfo</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">ActivityInfo</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">  <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">activityinfo</span><span style="color: #aa2211">-></span><span class="n">name</span><span class="p">(),</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">""</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">do you need a pointer here?</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/D10950#inline-52637" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">activityinfotest.cpp:73</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">QSignalSpy</span> <span style="color: #004012">spyName</span><span class="p">(</span><span class="n">activityinfo</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">ActivityInfo</span><span style="color: #aa2211">::</span><span class="n">nameChanged</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">  <span class="n">QVERIFY</span><span class="p">(</span><span class="n">spyName</span><span class="p">.</span><span class="n">count</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span style="color: #601200">0</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think you can create a macro that will can simplify the tests:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#define TEST_ATTRIBUTE(attributeName, accessorName, attributeType) \
{ \
  QFETCH(attributeType, attributeName); \
  QSignalSpy spy(activityinfo, &ActivityInfo::attributeName ## Changed); \
  QVERIFY(spy.count() == 0); \
  activityinfo->set ## accessorName(attributeName); \
  QVERIFY(spy.count() == 1); \
  QCOMPARE(activityinfo->##accessorName(), attributeName); \
}</pre></div>

<p style="padding: 0; margin: 8px;">and be used for all attributes?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R2 GCompris</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10950" rel="noreferrer">https://phabricator.kde.org/D10950</a></div></div><br /><div><strong>To: </strong>himanshuvishwakarma, GCompris, jjazeix, dmadaan, rudranilbasu, timotheegiet<br /><strong>Cc: </strong>KDE Edu, GCompris, harrymecwan, ganeshredcobra, nityanandkumar, echarruau, rahulyadav, narvaez, scagarwal, apol, timotheegiet, hkaelberer, jjazeix, bcoudoin<br /></div>