<table><tr><td style="">kossebau requested changes to this revision.<br />kossebau 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/D9344">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/D9344#339889" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D9344#339889</a>, <a href="https://phabricator.kde.org/p/rjvbb/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rjvbb</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>HtH...</p></div>
</blockquote>
<p>Thanks for the long explanation. Though I was hoping to get things documented for future KDevelop contributors (incl. our older selves) directly in the code.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>Ideally the strategy when it comes to project file names would be documented somewhere to developers in a comment.</p></blockquote>
<p>I'm tempted to say that the code should speak for itself, combined with runtime behaviour.</p></blockquote>
<p>Runtime behaviour does not tell whether this is the intended behaviour.. Bug or feature? And code only does what is possible on that level, by the concepts available in the used language and methods.<br />
What I am looking for is some human reader description of the intention. Most perfect when accompanied with unit test data as samples.</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/D9344#inline-88294">View Inline</a><span style="color: #4b4d51; font-weight: bold;">openprojectdialog.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; "> <span style="color: #aa2211">:</span> <span class="n">KAssistantDialog</span><span class="p">(</span> <span class="n">parent</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 class="n">m_projectDirUrl</span><span class="p">(</span><span class="n">QUrl</span><span class="p">())</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">,</span> <span class="n">m_urlIsDirectory</span><span class="p">(</span><span style="color: #304a96">false</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This will create a temporary QUrl object, which then gets passed as argument to the copy constructor of the QUrl of m_projectDirUrl, no? Not wanted here, or?<br />
Just remove the line and leave default constructor as applied by compiler do its job.</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/D9344#inline-88295">View Inline</a><span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:315</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">void</span> <span class="n">OpenProjectDialog</span><span style="color: #aa2211">::</span><span class="n">validateProjectName</span><span class="p">(</span> <span style="color: #aa4000">const</span> <span class="n">QString</span><span style="color: #aa2211">&</span> <span class="n">name</span> <span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The name <tt style="background: #ebebeb; font-size: 13px;">validateProjectName(</tt> promises the project name is validated.<br />
But the new code in this methods does also other stuff as sideeffect. This needs to be documented somewhere. Either in the method name (preferred), or as API 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/D9344#inline-88291">View Inline</a><span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:318</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">name</span> <span style="color: #aa2211">!=</span> <span class="n">m_projectName</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">bool</span> <span class="n">settingName</span> <span style="color: #aa2211">=</span> <span class="n">currentPage</span><span class="p">()</span> <span style="color: #aa2211">==</span> <span class="n">projectInfoPage</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">m_projectName</span> <span style="color: #aa2211">=</span> <span class="n">name</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">To me without context it is totally unclear why certain things (which actually in high level perspective?) are only done for that page, but not any other page? Perhaps this method should not be called otherwise? Or split into two separate methods, one to be called for the projectpage, the other for any other pages?</p>
<p style="padding: 0; margin: 8px;">In any case, given <tt style="background: #ebebeb; font-size: 13px;">settingName</tt> can be misunderstood on first read, let's rename to something less ambiguous, like <tt style="background: #ebebeb; font-size: 13px;">isDefiningProjectName</tt>.</p>
<p style="padding: 0; margin: 8px;">Personally also would favour adding brackets around the comparison, to speed up some human readers. So in total:</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);">const bool isDefiningProjectName = (currentPage() == projectInfoPage);</pre></div></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/D9344#inline-88296">View Inline</a><span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:332</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">QUrl</span> <span class="n">url</span><span class="p">(</span><span class="n">m_projectDirUrl</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// construct a version of the project name that's safe for use as a filename:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// TODO: do an additional replace of QDir::separator() with "@"?</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What is the definition of "safe"? Where can be conflicts? This is totally unclear to a reader of the actual code and code comments.</p>
<p style="padding: 0; margin: 8px;">Could this perhaps be factored out into some util class, which then also could get proper unit testing?</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/D9344#inline-88287">View Inline</a><span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:335-337</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">safeName</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">QRegularExpression</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"[</span><span style="color: #bb6622">\\\\</span><span style="color: #766510">/]"</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">safeName</span> <span style="color: #aa2211">=</span> <span class="n">safeName</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">QLatin1Char</span><span class="p">(</span><span style="color: #766510">':'</span><span class="p">),</span> <span class="n">QLatin1Char</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">safeName</span> <span style="color: #aa2211">=</span> <span class="n">safeName</span><span class="p">.</span><span class="n">replace</span><span class="p">(</span><span class="n">QRegExp</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"</span><span style="color: #bb6622">\\</span><span style="color: #766510">s"</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;"><p style="padding: 0; margin: 8px;">No need to assign to <tt style="background: #ebebeb; font-size: 13px;">safeName</tt>, <tt style="background: #ebebeb; font-size: 13px;">QString::replace(...)</tt> operates on same object and just returns reference for call chains (see also usage line above).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D9344">https://phabricator.kde.org/D9344</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, mwolff, kossebau<br /><strong>Cc: </strong>kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>