<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127489/">https://git.reviewboard.kde.org/r/127489/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 25th, 2016, 9:46 a.m. UTC, <b>Frederik Schwarzer</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There should be a pause state already, no? Why not just use that in the sound playing method?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I did not look at the sources carefully, but grepping it for "pause" gives quite some results. Does KBlocks already track the pause state in other places? Do you now add another instance of tracking?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other than that sublte feeling or irritation, the patch looks fine to me. :)</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is indeed already KBlocks_Game_State enum, but it is unused because no class actually keeps track of the current state. However, this made me realize that there is much simpler solution to the issue: KBlocks does indeed keep track whether the game is paused in the KeyboardPlayer class, ignoring all keyboard input when the game is paused. However, nobody told the KeyboardPlayer when the game was paused. Fixing this is trivial and the ignored keyboard input also means no sounds played.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thank you for having me take a closer look :)</p></pre>
<br />
<p>- Julian</p>
<br />
<p>On March 25th, 2016, 11:35 a.m. UTC, Julian Helfferich wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Games.</div>
<div>By Julian Helfferich.</div>
<p style="color: grey;"><i>Updated March 25, 2016, 11:35 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kblocks
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Testing the last patch to activate sounds in KBlocks, I realized another small bug: The sounds tied to keystrokes continue playing when the game is paused.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To fix this issue, KBlocksScene now keeps track of the state of the game. To this end, the bool mGameStarted is replaced with an enum class GameState taking the values Stopped, Running or Paused. When the state changes, the signal gameStateChanged() is emitted. This signal is connected to the slot updateSoundsEnabled() which sets the sound to enabled if GameState is Running and sounds are enabled in the system settings.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In summary, I think the approach is more elaborate than what would be necessary, but more preferable to change the sound separately in startGame(), stopGame() and pauseGame().</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Played the game.</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Enabled and disabled the sound while game is running.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Enabled and disabled the sound when game is paused.</li>
</ul></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>KBlocksWin.cpp <span style="color: grey">(522bfdd)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127489/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>