<table><tr><td style="">sitter planned changes to this revision.<br />sitter added inline comments.
</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/D17560">View Revision</a></tr></table><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/D17560#inline-106719">View Inline</a><span style="color: #4b4d51; font-weight: bold;">aacid</span> wrote in <span style="color: #4b4d51; font-weight: bold;">toplevel.cpp:236</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">this is wrong, you moved the language.isEmpty up and now the else triggers in cases it should not trigger.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Hm. That is true. Truth be told even after pointing that out it still took me way to long to see the problem -.-</p>

<p style="padding: 0; margin: 8px;">I think I wanted to avoid the extra level of nesting. With that motivation in mind, what do you think about splitting the if</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);">if (soundEnabled && language.isEmpty())
{
  const QStringList &systemLanguages = KLocalizedString::languages();
  ....
}

if (!soundEnabled)
{
  soundOff();
  language = QString();
}</pre></div>

<p style="padding: 0; margin: 8px;">In my mind the two code blocks aren't two sides of the same coin. One is language fallback-handling the other is the unrelated disabling of all sound. The only thing that links them is the fact that the only thing language is used for is to figure out what sound set to use.</p>

<p style="padding: 0; margin: 8px;">I don't mind particularly though, so if you'd like to have</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);">if (soundEnabled) {
  if (language.isEmpty()) { ... }
} else { ... }</pre></div>

<p style="padding: 0; margin: 8px;">I'm fine with that too</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R418 KTuberling</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17560">https://phabricator.kde.org/D17560</a></div></div><br /><div><strong>To: </strong>sitter, aacid, KDE Games<br /><strong>Cc: </strong>kde-games-devel<br /></div>