<table><tr><td style="">ahiemstra 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/D27544">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/D27544#inline-159205">View Inline</a><span style="color: #4b4d51; font-weight: bold;">engine.cpp:614</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">QString</span> <span class="n">identifiedLink</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 style="color: #aa2211">&</span> <span class="n">payloadToIdentify</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">payloadToIdentify</span><span class="p">[</span><span class="n">entry</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">QStringList</span><span style="color: #aa2211">&</span> <span class="n">payloads</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">payloads</span><span class="p">[</span><span class="n">entry</span><span class="p">];</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Code style: & attaches to the name, not the type. (Yes I hate it too).</p>
<p style="padding: 0; margin: 8px;">There's a few instances of this around.</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/D27544#inline-159204">View Inline</a><span style="color: #4b4d51; font-weight: bold;">engine.cpp:621</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: #74777d">// Next simplest option, filename is the same but in a different folder</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">QStringRef</span><span style="color: #aa2211">&</span> <span class="n">fileName</span> <span style="color: #aa2211">=</span> <span class="n">payloadToIdentify</span><span class="p">.</span><span class="n">splitRef</span><span class="p">(</span><span class="n">QChar</span><span style="color: #aa2211">::</span><span class="n">fromLatin1</span><span class="p">(</span><span style="color: #766510">'/'</span><span class="p">)).</span><span class="n">last</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 class="n">QString</span><span style="color: #aa2211">&</span> <span style="color: #a0a000">payload</span> <span class="p">:</span> <span class="n">payloads</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;">This makes a reference to a temporary which I'm not sure is a good idea. Since you are already using splitRef, just make a copy.</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/D27544#inline-159207">View Inline</a><span style="color: #4b4d51; font-weight: bold;">engine.cpp:631</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: #74777d">// Least simple option, no match - ask the user to pick (and if we still haven't got one... that's us done, no installation)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Question</span><span style="color: #aa2211">*</span> <span class="n">question</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">Question</span><span class="p">(</span><span class="n">Question</span><span style="color: #aa2211">::</span><span class="n">SelectFromListQuestion</span><span class="p">,</span> <span style="color: #aa4000">this</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">question</span><span style="color: #aa2211">-></span><span class="n">setTitle</span><span class="p">(</span><span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Pick Update Item"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This object will linger until the engine is destroyed, which seems like a suboptimal thing. Maybe better to do:</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);">auto question = std::make_unique<Question>(Question::SelectFromListQuestion);</pre></div>
<p style="padding: 0; margin: 8px;">then it will be automatically cleaned up once you exit the scope.</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/D27544#inline-159203">View Inline</a><span style="color: #4b4d51; font-weight: bold;">engine.cpp:644</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_installation</span><span style="color: #aa2211">-></span><span class="n">install</span><span class="p">(</span><span class="n">theEntry</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: #74777d">// As the serverside data may change before next time this is called, even in the same session,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You may want to log something in the else here, at least then it will be possible to figure out why an update is not happening.</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/D27544#inline-159199">View Inline</a><span style="color: #4b4d51; font-weight: bold;">EntryDetails.qml:101</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; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #004012">component</span><span class="p">.</span><span style="color: #004012">downloadCount</span> <span style="color: #aa2211">==</span> <span style="color: #601200">1</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #004012">newStuffModel</span><span class="p">.</span><span style="color: #004012">installItem</span><span class="p">(</span><span style="color: #004012">component</span><span class="p">.</span><span style="color: #004012">index</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">newStuffModel</span><span class="p">.</span><span style="color: #004012">installItem</span><span class="p">(</span><span style="color: #004012">component</span><span class="p">.</span><span style="color: #004012">index<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">1</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That "1" here is a bit mysterious, what does it actually mean?</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/D27544#inline-159201">View Inline</a><span style="color: #4b4d51; font-weight: bold;">quickitemsmodel.h:154</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; "><span style="color: #74777d"> * @param index The index of the item to install or update</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @param linkId The download item to install. If this is -1, it is assumed to be an update with an unknown payload, and a number of heuristics will be applied by the engine</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @see Engine::downloadLinkLoaded implementation for details</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Rather than using -1, you could add an enum that defines these values a bit more, like:</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);">enum LinkId {
AutoDetectLink = -1,
FirstItem = 1
}</pre></div>
<p style="padding: 0; margin: 8px;">You can then also expose that to QML to avoid the magical 1 up there.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R304 KNewStuff</div></div></div><br /><div><strong>BRANCH</strong><div><div>fix-update (branched from master)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27544">https://phabricator.kde.org/D27544</a></div></div><br /><div><strong>To: </strong>leinir, KNewStuff, Frameworks, Plasma, ngraham, apol, Discover Software Store<br /><strong>Cc: </strong>ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns<br /></div>