<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</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/D26448">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/D26448#inline-149835">View Inline</a><span style="color: #4b4d51; font-weight: bold;">krecentfilesmenu.cpp:99</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">setIcon</span><span class="p">(</span><span class="n">QIcon</span><span style="color: #aa2211">::</span><span class="n">fromTheme</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"document-open-recent"</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">fileName</span> <span style="color: #aa2211">=</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"%1/%2recentfiles"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">writableLocation</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">AppDataLocation</span><span class="p">),</span> <span class="n">QCoreApplication</span><span style="color: #aa2211">::</span><span class="n">applicationName</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_settings</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QSettings</span><span class="p">(</span><span class="n">fileName</span><span class="p">,</span> <span class="n">QSettings</span><span style="color: #aa2211">::</span><span class="n">Format</span><span style="color: #aa2211">::</span><span class="n">IniFormat</span><span class="p">,</span> <span style="color: #aa4000">this</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">maybe %2_recentfiles to improve readability?</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/D26448#inline-149836">View Inline</a><span style="color: #4b4d51; font-weight: bold;">krecentfilesmenu.cpp:189</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">delete</span> <span style="color: #aa2211">*</span><span class="n">it</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">m_entries</span><span class="p">.</span><span class="n">erase</span><span class="p">(</span><span class="n">it</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">rebuildMenu</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">How does this work, given that <tt style="background: #ebebeb; font-size: 13px;">it</tt> is a const_iterator?<br />
I smell compilation errors on some compilers/OS/debug-or-release mode.</p>

<p style="padding: 0; margin: 8px;">findEntry is only used for add/remove, I guess it shouldn't return a const_iterator but an iterator.</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/D26448#inline-149338">View Inline</a><span style="color: #4b4d51; font-weight: bold;">nicolasfella</span> wrote in <span style="color: #4b4d51; font-weight: bold;">krecentfilesmenu.cpp:85</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">New entries are pushed to the front, which works in constant time for std::list and linear time for std::vector<br />
When the maximum is reached the last element is removed which works in constant time for both.</p>

<p style="padding: 0; margin: 8px;">We could turn it around and add new items to the back and iterate reverse, but then removing the front item would take linear time for std::vector</p>

<p style="padding: 0; margin: 8px;">Given that n is small (<10 by default) it shouldn't matter much anyway</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's the theory as it was taught in schools years ago.</p>

<p style="padding: 0; margin: 8px;">In practice, moving N pointers that are in the same CPU cache line (because they are in a std::vector) is faster than findEntry accessing nodes all over memory (because of std::list), which means loading N different areas of memory into the CPU cache.<br />
Because the element type is a pointer (and not a complex value class), that "linear time" is a single memmove, hopefully.</p>

<p style="padding: 0; margin: 8px;">Anyway, as you say, it doesn't really matter so I'm happy to let it go, I just wanted to say that I still do believe std::vector is better suited :)</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/D26448#inline-149207">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">krecentfilesmenu.cpp:150</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If we're rewriting this thing anyway, can we please make sure it does not block application startup checking if those files exist, as can happen if you had opened files on an NFS mount before.</p>

<p style="padding: 0; margin: 8px;">Either do it only when the menu is opened (aboutToShow) and/or asynchronously.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Blocking in aboutToShow is even worse in terms of user experience, isn't it?</p>

<p style="padding: 0; margin: 8px;">If the NFS mount was manual, this will just fail and not block, anyway.<br />
But with automounting this can go very wrong indeed.</p>

<p style="padding: 0; margin: 8px;">You know what I think.... don't mount network drives, use KIO :)</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/D26448#inline-148907">View Inline</a><span style="color: #4b4d51; font-weight: bold;">cfeck</span> wrote in <span style="color: #4b4d51; font-weight: bold;">krecentfilesmenu.cpp:215</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Should we truncate the current list if the new max items is smaller?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">^ this comment hasn't been addressed yet.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R236 KWidgetsAddons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D26448">https://phabricator.kde.org/D26448</a></div></div><br /><div><strong>To: </strong>nicolasfella, Frameworks, dfaure<br /><strong>Cc: </strong>broulik, elvisangelaccio, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>