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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Rereading the patch and the related code once more, my personal opinion now is: this logic has been confusing before and only gets more confusing with the proposed patch.
  
  This needs refactoring. e.g. by moving the actual projectfile name generation/definition into a new dedicated class caring only for this very aspect. Which then can also be properly unit tested to cover all the conditions which can be expected.
  
  Not the most chilling review comment one likes to get, I know myself. It's the disadvantage of being the person who wants to add a feature to existing convoluted code :)
  But the final result should be code which we all should feel well with and which helps to keep/make kdevelop's code base maintainable and easier to extend.
  
  The current patch I still cannot oversee (though also due to the existing code), so I would have to grab the -1 sign for now if on the jury.

INLINE COMMENTS

> rjvbb wrote in openprojectdialog.cpp:277
> 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.

Re: related discussion, I might have misunderstood some of the comments in this review,, could not find it back on a re-read.

Thanks for adding the comment to the code, but I am still wondering by the code how this situation can happen. If a user selected an existing file with that extension and thus m_url is "/path/to/projectSrc/projectSrc.kdev", the very

  if( !urlInfo.isDir ) {
      m_url = m_url.adjusted(QUrl::StripTrailingSlash | QUrl::RemoveFilename);
  }

earlier should handle this, no? Or does this not do what is expected (in some Qt version)? Or csan the incoming path have a trailing slash already which spoils dropping the last path element by QUrl::RemoveFilename?
Even more, how would we actually end in this branch? It's condition is

  if( urlInfo.isDir || urlInfo.extension != ShellExtension::getInstance()->projectFileExtension() )

so we only can arrive here with a "/path/to/projectSrc/projectSrc.kdev" if there is some existing directory selected which has a suffix ".kdev4". KDevelop itself does not generate such directories, or?
Only the personal project directory, which is hidden though in the file select dialog, no? (Not sure what is done on none unixoid systems) That might be something to give special handling to catch in case, but then rather at the begin of the if branch. Doing any bad case handling at this point seems to be like we missed something before.

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/20181020/44c129f6/attachment.html>