D9344: [KDevelop] : [fixed] consistent use of the project name allowing to create multiple projects in a single source tree

[René J.V. Bertin]

rjvbb marked 7 inline comments as done.
rjvbb added a comment.


  Where in the code would you want a documentation of the intent of this change?
  The best place might be a in the handbook or the project import wizard GUI ... but we're in string freeze, no?

INLINE COMMENTS

> kossebau wrote in openprojectdialog.cpp:277
> When can this situation happen? After all `m_url` is handled above with
> 
>   if( !urlInfo.isDir ) {
>       m_url = m_url.adjusted(QUrl::StripTrailingSlash | QUrl::RemoveFilename);
>   }
> 
> 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?
> 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.

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.

> kossebau wrote in openprojectdialog.cpp:315
> The name `validateProjectName(` promises the project name is validated.
> 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.

the method originally *set* the project name, and then validated the project info - the name was thus not very appropriate already.
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.

> kossebau wrote in openprojectdialog.cpp:318
> 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?
> 
> In any case, given `settingName` can be misunderstood on first read, let's rename to something less ambiguous, like `isDefiningProjectName`.
> 
> Personally also would favour adding brackets around the comparison, to speed up some human readers. So in total:
> 
>   const bool isDefiningProjectName = (currentPage() == projectInfoPage);

Normally I'd have used brackets, I think I omitted them here to avoid requests to reduce redundancy...

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.

I did simplify the function a bit: I had overlooked that there's no reason to calculate the local `url` and `safeName` variables when `!settingName` (now `isEnteringProjectName`).

> kossebau wrote in openprojectdialog.cpp:332
> What is the definition of "safe"? Where can be conflicts? This is totally unclear to a reader of the actual code and code comments.
> 
> Could this perhaps be factored out into some util class, which then also could get proper unit testing?

I supposed `KDevelop::Path` could have a method that creates a "safe" instance from an input QString/QUrl but that'd be a separate change.
Are you aware of other places where filenames are sanitised in a similar fashion?

> kossebau wrote in openprojectdialog.cpp:335-337
> No need to assign to `safeName`, `QString::replace(...)` operates on same object and just returns reference for call chains (see also usage line above).

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).

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D9344

To: rjvbb, #kdevelop, mwolff, kossebau
Cc: kossebau, arrowd, mschwarz, kfunk, mwolff, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181016/83fd55c1/attachment-0001.html>