<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<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/D10363" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Right, apart from the coding issues I found, the bigger issue is that this will not work, xdg templates are simple files I guess, not desktop files.</p>
<p>In case you're wondering, the main reason I killed the support for normal files was:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">we need translated names, which just isn't available with just a filename. But OK, in the user's dir, we can assume they understand their own filenames ;)</li>
<li class="remarkup-list-item">a "nice" display name is better than just a filename.</li>
</ul>
<p>I don't mind some kind of "xdg compatibility" with a lower feature set, but I wouldn't want this code to suddenly support normal files in the KDE dirs, it will just make people (e.g. distros who like to add their own templates) ignore those issues and go for solutions that don't support nice and translated display names.</p>
<p>So maybe the code shouldn't be adding to the installedTemplates variable, but rather doing its own parsing of the xdg template dir, separately from everything else.</p>
<p>Remind me though, do we have a GUI for users to add templates into the already-supported (non-xdg) user-specific template dir, creating the .desktop file for it on the fly?</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/D10363#inline-48399" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:896</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(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringList</span></span><span class="bright"> </span><span class="n"><span class="bright">installedTemplates</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">locateAll</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">GenericDataLocation</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"templates"</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">LocateDirectory</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">QStringList</span></span><span class="bright"> </span><span class="n"><span class="bright">installedTemplates</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span><span class="bright"> </span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">locateAll</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">GenericDataLocation</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"templates"</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">QStandardPaths</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">LocateDirectory</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">}</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Qt does not provide as easy way to recieve the xdg dir for templates so we have to find it by our own</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">xdgUserDirs</span> <span style="color: #aa2211">=</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">locate</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">ConfigLocation</span><span class="p">,</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"user-dirs.dirs"</span><span class="p">),</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">LocateFile</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">All your new code should be in #ifdef Q_OS_UNIX, it is not applicable on e.g. Windows.</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/D10363#inline-48398" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:899</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">QFile</span> <span style="color: #004012">xdgUserDirsFile</span><span class="p">(</span><span class="n">xdgUserDirs</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">xdgUserDirsFile</span><span class="p">.</span><span class="n">exists</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span class="n">xdgUserDirsFile</span><span class="p">.</span><span class="n">open</span><span class="p">(</span><span class="n">QIODevice</span><span style="color: #aa2211">::</span><span class="n">ReadOnly</span> <span style="color: #aa2211">|</span> <span class="n">QIODevice</span><span style="color: #aa2211">::</span><span class="n">Text</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">QTextStream</span> <span class="n">in</span><span class="p">(</span><span style="color: #aa2211">&</span><span class="n">xdgUserDirsFile</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">remove the exists() check, open will fail if it doesn't exist, anyway, so we might as well save one syscall.</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/D10363#inline-48400" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:904</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">if</span> <span class="p">(</span><span class="n">line</span><span class="p">.</span><span class="n">startsWith</span><span class="p">(</span><span style="color: #766510">"XDG_TEMPLATES_DIR="</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">QString</span> <span class="n">xdgTemplates</span> <span style="color: #aa2211">=</span> <span class="n">line</span><span class="p">.</span><span class="n">mid</span><span class="p">(</span><span style="color: #601200">19</span><span class="p">,</span> <span class="n">line</span><span class="p">.</span><span class="n">size</span><span class="p">()</span><span style="color: #aa2211">-</span><span style="color: #601200">20</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">xdgTemplates</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">QString</span><span class="p">(</span><span style="color: #766510">"$HOME/"</span><span class="p">),</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">locate</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">HomeLocation</span><span class="p">,</span> <span class="n">QString</span><span class="p">(),</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">LocateDirectory</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">the 2nd arg isn't necessary, just do line.mid(19)</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/D10363#inline-48401" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:905</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">xdgTemplates</span> <span style="color: #aa2211">=</span> <span class="n">line</span><span class="p">.</span><span class="n">mid</span><span class="p">(</span><span style="color: #601200">19</span><span class="p">,</span> <span class="n">line</span><span class="p">.</span><span class="n">size</span><span class="p">()</span><span style="color: #aa2211">-</span><span style="color: #601200">20</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">xdgTemplates</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">QString</span><span class="p">(</span><span style="color: #766510">"$HOME/"</span><span class="p">),</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">locate</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">HomeLocation</span><span class="p">,</span> <span class="n">QString</span><span class="p">(),</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">LocateDirectory</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QDir</span> <span style="color: #004012">xdgTemplatesDir</span><span class="p">(</span><span class="n">xdgTemplates</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">QStringLiteral("$HOME/")</p>
<p style="padding: 0; margin: 8px;">No need to use QStandardPaths to get the home dir (that's... historical), use QDir::homePath() directly.</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/D10363#inline-48403" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:910</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="p">}</span> <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">xdgTemplates</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 class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">wait, what? Interesting that this compiles, it's not the Qt way to clear a string (that would be .clear()). But just kill the line (and therefore the else), it's unnecessary, the variable is going out of scope anyway.</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/D10363#inline-48404" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">knewfilemenu.cpp:915</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="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">xdgUserDirsFile</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="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">not needed, the destructor closes.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D10363" rel="noreferrer">https://phabricator.kde.org/D10363</a></div></div><br /><div><strong>To: </strong>mmustac, Frameworks, dfaure<br /><strong>Cc: </strong>broulik, ngraham, michaelh<br /></div>