<table><tr><td style="">pino 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/D21661">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/D21661#476607" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21661#476607</a>, <a href="https://phabricator.kde.org/p/sredman/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@sredman</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><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/D21661#476571" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21661#476571</a>, <a href="https://phabricator.kde.org/p/pino/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@pino</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>It seems the snoretoast library provides a <tt style="background: #ebebeb; font-size: 13px;">SnoreToasts</tt> class to do this instead of spawning an helper tool, what about using it instead?</p></div>
</blockquote>
<p><a href="https://phabricator.kde.org/p/vonreth/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@vonreth</a> can probably explain better, but basically the situation as I understand it is on Windows you need to be installed in a special place and registered with the OS in order to show notifications. Since KNotifications is a library, an app using it can't (feasibly) be properly registered with the OS. It is possible we could come up with some complicated solution which would require every KNotification-using app to do some special and probably difficult to understand change to support Windows. Or we can have SnoreNotify.exe take care of all that nonsense for us. Note that, up to this point, there have been no special KNotifications changes to the generic KDE Connect codebase to make this work, just some tweaks to the Windows installer to pull in SnoreToast.</p></div>
</blockquote>
<p>IMHO this, plus what <a href="https://phabricator.kde.org/p/vonreth/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@vonreth</a> adds later on, is exactly the kind of information that is important enough to be explained at least either as comment in the sources, or as commit message. This way people know why this approach was used, and they will not try to change things later on without dealing with the same situation.</p>
<p>Furthermore:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">I still do think that is better to remove the cached image of a notification when removing it. Yes, they are stored in a QTemporaryDir directory, so they will be deleted when the application exists, although they will pile up and take more and more space. This is not a problem for short-living applications, but it is for long-running application, for example:<ul class="remarkup-list">
<li class="remarkup-list-item">a media player showing the popup notification of a new song with its album cover, so every 3/4/5 minutes</li>
<li class="remarkup-list-item">a IM application showing messages from other users with their avatars when a message is received, so potentially even every few seconds</li>
</ul></li>
<li class="remarkup-list-item">regarding the notification image: so far only the pixmap is set, although there are other ways to set the "artwork" to use for a notification: for example the <tt style="background: #ebebeb; font-size: 13px;">IconName</tt> in the notifyrc configuration, or the iconName proeprty in <tt style="background: #ebebeb; font-size: 13px;">KNotifyConfig</tt>; check what the NotifyByPopup plugin does, for example, to get them, and what the NotifyByAndroid plugin does to get an image from a theme icon name</li>
</ul></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/D21661#inline-121994">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:112</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">Qt5::Core</tt> is not needed here; if we really want to be pedantic, it ought to be added unconditionally (however that would be a different patch than this one)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">still there</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/D21661#inline-121991">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:48-49</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Qt5Core is already pretty much required to build anything using Qt5, so looking for it in this place is a no-op (because it was already found)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">still there</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/D21661#inline-122268">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:29</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"><QString></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"><QTemporaryDir></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"><QLoggingCategory></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">already included in the .h file</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/D21661#inline-122267">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:31</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"><QLoggingCategory></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"><QLocalServer></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"><QLocalSocket></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">already included in the .h file</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/D21661#inline-122269">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:58</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: #aa4000">const</span> <span class="n">QByteArray</span> <span class="n">rawData</span> <span style="color: #aa2211">=</span> <span class="n">sock</span><span style="color: #aa2211">-></span><span class="n">readAll</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">sock</span><span style="color: #aa2211">-></span><span class="n">close</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">data</span> <span style="color: #aa2211">=</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">closing is good, although the socket object is still leaked</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/D21661#inline-122280">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:63</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">QMap</span><span style="color: #aa2211"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span style="color: #aa2211">></span> <span class="n">map</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">str</span> <span class="p">:</span> <span class="n">data</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">";"</span><span class="p">)))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">index</span> <span style="color: #aa2211">=</span> <span class="n">str</span><span class="p">.</span><span class="n">indexOf</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;"><ul class="remarkup-list">
<li class="remarkup-list-item">splitRef here to not allocate strings that are not used anyway (since they are split again later on)</li>
<li class="remarkup-list-item">you are using a C++11 for loop on a Qt container, and this will detach it -- you need to store the container as variable:</li>
</ul>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="c++" 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);"><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">parts</span> <span style="color: #aa2211">=</span> <span class="n">data</span><span class="p">.</span><span class="n">splitRef</span><span class="p">(...);</span>
<span style="color: #aa4000">for</span> <span class="p">(...</span> <span style="color: #aa2211">:</span> <span class="n">parts</span><span class="p">)</span></pre></div>
<ul class="remarkup-list">
<li class="remarkup-list-item">QLatin1Char is better than QStringLiteral here (since you have a single character as separator)</li>
</ul></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/D21661#inline-122281">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:64</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: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">str</span> <span class="p">:</span> <span class="n">data</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">";"</span><span class="p">)))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">index</span> <span style="color: #aa2211">=</span> <span class="n">str</span><span class="p">.</span><span class="n">indexOf</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 style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">map</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">str</span><span class="p">.</span><span class="n">mid</span><span class="p">(</span><span style="color: #601200">0</span><span class="p">,</span> <span class="n">index</span><span class="p">),</span> <span class="n">str</span><span class="p">.</span><span class="n">mid</span><span class="p">(</span><span class="n">index</span> <span style="color: #aa2211">+</span> <span style="color: #601200">1</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">ditto, QStringLiteral -> QLatin1Char</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/D21661#inline-122270">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:70</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">KNotification</span> <span style="color: #aa2211">*</span><span class="n">notification</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">nullptr</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">it</span> <span style="color: #aa2211">=</span> <span class="n">m_notifications</span><span class="p">.</span><span class="n">find</span><span class="p">(</span><span class="n">id</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">it</span> <span style="color: #aa2211">!=</span> <span class="n">m_notifications</span><span class="p">.</span><span class="n">end</span><span class="p">())</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">constFind</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/D21661#inline-122271">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:71</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: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">it</span> <span style="color: #aa2211">=</span> <span class="n">m_notifications</span><span class="p">.</span><span class="n">find</span><span class="p">(</span><span class="n">id</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">it</span> <span style="color: #aa2211">!=</span> <span class="n">m_notifications</span><span class="p">.</span><span class="n">end</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">notification</span> <span style="color: #aa2211">=</span> <span class="n">it</span><span class="p">.</span><span class="n">value</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">constEnd</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/D21661#inline-122279">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:77</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: #aa4000">switch</span> <span class="p">(</span><span class="n">snoreAction</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">case</span> <span class="n">SnoreToastActions</span><span style="color: #aa2211">::</span><span class="n">Actions</span><span style="color: #aa2211">::</span><span style="color: #a0a000">Clicked</span> <span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qCDebug</span><span class="p">(</span><span class="n">LOG_KNOTIFICATIONS</span><span class="p">)</span> <span style="color: #aa2211"><<</span> <span style="color: #766510">" User clicked on the toast."</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">no space between the enum and the colon (applies to the other lines too)</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/D21661#inline-122272">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:115</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">server</span><span class="p">.</span><span class="n">close</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">iconDir</span><span class="p">.</span><span class="n">remove</span><span class="p">();</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;">this is not needed, the destructor of QTemporaryDir will do it by default</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/D21661#inline-122274">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:126</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">QStringList</span> <span class="n">arguments</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">iconPath</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">move this variable directly in the code block where it is used</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/D21661#inline-122273">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:131-132</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">arguments</span> <span style="color: #aa2211"><<</span> <span class="n">notification</span><span style="color: #aa2211">-></span><span class="n">title</span><span class="p">();</span>
</div><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 style="color: #aa4000">else</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">arguments</span> <span style="color: #aa2211"><<</span> <span class="n">qApp</span><span style="color: #aa2211">-></span><span class="n">applicationDisplayName</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">coding style: they go in the same line</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/D21661#inline-122276">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:151</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">m_notifications</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">notification</span><span style="color: #aa2211">-></span><span class="n">id</span><span class="p">(),</span> <span class="n">notification</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">proc</span><span style="color: #aa2211">-></span><span class="n">start</span><span class="p">(</span><span class="n">program</span><span class="p">,</span> <span class="n">arguments</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">better have this as very last operation done, so early returns will not have this notification in <tt style="background: #ebebeb; font-size: 13px;">m_notifications</tt></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/D21661#inline-122275">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:152</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">m_notifications</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span><span class="n">notification</span><span style="color: #aa2211">-></span><span class="n">id</span><span class="p">(),</span> <span class="n">notification</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">proc</span><span style="color: #aa2211">-></span><span class="n">start</span><span class="p">(</span><span class="n">program</span><span class="p">,</span> <span class="n">arguments</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">most probably check whether starting the process succeeded, and if not call <tt style="background: #ebebeb; font-size: 13px;">finish(notification)</tt> and return earlier</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/D21661#inline-122277">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:155</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">connect</span><span class="p">(</span><span class="n">proc</span><span class="p">,</span> <span class="n">QOverload</span><span style="color: #aa2211"><</span><span style="color: #aa4000">int</span><span class="p">,</span> <span class="n">QProcess</span><span style="color: #aa2211">::</span><span class="n">ExitStatus</span><span style="color: #aa2211">>::</span><span class="n">of</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">QProcess</span><span style="color: #aa2211">::</span><span class="n">finished</span><span class="p">),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">[</span><span style="color: #aa2211">=</span><span class="p">](</span><span style="color: #aa4000">int</span> <span class="n">exitCode</span><span class="p">,</span> <span class="n">QProcess</span><span style="color: #aa2211">::</span><span class="n">ExitStatus</span> <span class="n">exitStatus</span><span class="p">){</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">proc</span><span style="color: #aa2211">-></span><span class="n">deleteLater</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">IMHO it is better to check whether the process exited correctly, and its return status was a success, finish'ing the notification in case of failure</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/D21661#inline-122278">View Inline</a><span style="color: #4b4d51; font-weight: bold;">notifybysnore.cpp:169</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">QProcess</span> <span style="color: #aa2211">*</span><span class="n">proc</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QProcess</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QStringList</span> <span class="n">arguments</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">considering here the notification is being closed and thus checking what the tool does has almost no effect on the flow, then there is no need to manually create a QProcess object:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">collect the arguments (like you already do)</li>
<li class="remarkup-list-item">use the static <tt style="background: #ebebeb; font-size: 13px;">QProcess::startDetached(program, args)</tt></li>
</ul></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/D21661#inline-122248">View Inline</a><span style="color: #4b4d51; font-weight: bold;">brute4s99</span> wrote in <span style="color: #4b4d51; font-weight: bold;">notifybysnore.h:44</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I referred to this: <a href="https://woboq.com/blog/qmap_qhash_benchmark.html" class="remarkup-link" target="_blank" rel="noreferrer">https://woboq.com/blog/qmap_qhash_benchmark.html</a></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">All the other <tt style="background: #ebebeb; font-size: 13px;">KNotificationPlugin</tt>s in knotification use QHash for indexing the <tt style="background: #ebebeb; font-size: 13px;">KNotification</tt> objects, FWIW, so at least using QHash would keep a bit of consistency.</p></div></div></div></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/D21661">https://phabricator.kde.org/D21661</a></div></div><br /><div><strong>To: </strong>brute4s99, broulik, sredman, vonreth, albertvaka<br /><strong>Cc: </strong>nicolasfella, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns<br /></div>