<table><tr><td style="">rjvbb marked 7 inline comments as done.<br />rjvbb 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/D9344">View Revision</a></tr></table><br /><div><div><p>Where in the code would you want a documentation of the intent of this change?<br />
The best place might be a in the handbook or the project import wizard GUI ... but we're in string freeze, no?</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-88303">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:277</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">When can this situation happen? After all <tt style="background: #ebebeb; font-size: 13px;">m_url</tt> is handled above with</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);">if( !urlInfo.isDir ) {
    m_url = m_url.adjusted(QUrl::StripTrailingSlash | QUrl::RemoveFilename);
}</pre></div>

<p style="padding: 0; margin: 8px;">Do people have directories using the projectFileExtension in the dir name? Or would they select the hidden directories with the personal kdevelop project data? Why should the global project filename not be set in this case?<br />
Please add a code comment to make this clear. There has been some related discussion in the review comments, but on a few minutes read I have not grasped this logic, so at least for code readers like me some code comment explanation is needed.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">As a matter of fact I cannot find (really) related discussion. I hope the comment I'm adding makes clear enough what the code does and why.</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;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:315</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">the method originally *set* the project name, and then validated the project info - the name was thus not very appropriate already.<br />
My change makes the method do an actual validation of the project name - the term implies that checks are made that the name is actually valid.</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;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:318</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Normally I'd have used brackets, I think I omitted them here to avoid requests to reduce redundancy...</p>

<p style="padding: 0; margin: 8px;">I mostly agree that the underlying design is unclear here; I had not expected at all that this method could be called before the dialog for entering a project name was posted. However splitting it up may not be straightforward (it's used only as a slot connected to a single signal) and would probably lead to some code duplication.</p>

<p style="padding: 0; margin: 8px;">I did simplify the function a bit: I had overlooked that there's no reason to calculate the local <tt style="background: #ebebeb; font-size: 13px;">url</tt> and <tt style="background: #ebebeb; font-size: 13px;">safeName</tt> variables when <tt style="background: #ebebeb; font-size: 13px;">!settingName</tt> (now <tt style="background: #ebebeb; font-size: 13px;">isEnteringProjectName</tt>).</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-88296">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:332</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I supposed <tt style="background: #ebebeb; font-size: 13px;">KDevelop::Path</tt> could have a method that creates a "safe" instance from an input QString/QUrl but that'd be a separate change.<br />
Are you aware of other places where filenames are sanitised in a similar fashion?</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;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">openprojectdialog.cpp:335-337</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><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 style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done, but in this case I found the additional assign increased readability (= I'm not convinced by any of the line folding approaches I tried).</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>