<table><tr><td style="">aacid 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/D11891">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
          color: #6B748C;
          font-style: italic;
          margin: 4px 0 12px 0;
          padding: 8px 12px;
          background-color: #F8F9FC;">
<div style="font-style: normal;
          padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D11891#239437" style="background-color: #e7e7e7;
          border-color: #e7e7e7;
          border-radius: 3px;
          padding: 0 4px;
          font-weight: bold;
          color: black;text-decoration: none;">D11891#239437</a>, <a href="https://phabricator.kde.org/p/rjvbb/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;">@rjvbb</a> wrote:</div>
<div style="margin: 0;
          padding: 0;
          border: 0;
          color: rgb(107, 116, 140);"><p>This is about better and more concise English. The queued connection is the indirect explanation why the patch is necessary, and thus comes after the direct explanation (the fact that there may be pending signals). Think of it as a courtesy to people who want to get to the point first and maybe deal with the finer detail later.</p>

<p>The problem with this whole comment is that it's long and not very easy to follow for devs who are not (very) familiar with the code already (and those who are might not need all the detail). Rereading it with after-bedtime eyes I think you should probably just leave only the 1st sentence. The explanation why you can end up "here" after close was called could be put in the commit message, or as a "warning" above the connect() call that creates the connection.</p></div>
</blockquote>

<p>I disagree, i've read that code lots of times and it took me finding a semi-reproducible case to figure out what was wrong, so having a comment helps people that are familiar with it.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Come to think of it, your patch could take the form below because there is already a check of <tt style="background: #ebebeb; font-size: 13px;">notification</tt>:</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 (Q_UNLIKELY(!notification)) {
    return;
} else if ((notification->flags() & KNotification::LoopSound)) {
    m->play();
    return;
}</pre></div>

<p>Maybe merge your comment with the one about "if the sound is short enough" because I from what I understand that describes more or less the same situation.</p></blockquote>

<p>No, it describes a different situation, it's about</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);">m->enqueue(soundURL);</pre></div>

<p>not having had time to finish and thus onAudioSourceChanged not triggering.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R289 KNotifications</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11891">https://phabricator.kde.org/D11891</a></div></div><br /><div><strong>To: </strong>aacid, Frameworks, cullmann, rjvbb<br /><strong>Cc: </strong>cfeck, rjvbb, mpyne, michaelh, ngraham<br /></div>