<table><tr><td style="">kossebau 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/D11389">View Revision</a></tr></table><br /><div><div><p>Thanks for first review :)</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/D11389#inline-56074">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mtijink</span> wrote in <span style="color: #4b4d51; font-weight: bold;">mpriscontrolplugin.cpp:118</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This loop can be replaced with <tt style="background: #ebebeb; font-size: 13px;">std::find_if</tt> (also elsewhere in the code).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I considered that, but did not find an example with QHash, so was unsure if that works in all versions of Qt that should be supported. Also from what i sketched did it not really simplify the code. But will try, then we/you can compare & comment :)</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/D11389#inline-56078">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mtijink</span> wrote in <span style="color: #4b4d51; font-weight: bold;">mpriscontrolplugin.cpp:230</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Instead of using raw <tt style="background: #ebebeb; font-size: 13px;">delete</tt>s here, it's better to use <tt style="background: #ebebeb; font-size: 13px;">std::unique_ptr</tt> (or a Qt equivalent?).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You are right, it makes sense to move memory management completely over to MprisPlayer instances (shared pointer though, given MprisPlayer is moved around by value). To shy before to go the full path :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R224 KDE Connect</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11389">https://phabricator.kde.org/D11389</a></div></div><br /><div><strong>To: </strong>kossebau, KDE Connect, mtijink<br /><strong>Cc: </strong>mtijink<br /></div>