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

[Friedrich W. H. Kossebau]

kossebau requested changes to this revision.
kossebau added a comment.
This revision now requires changes to proceed.


  In D9344#339889 <https://phabricator.kde.org/D9344#339889>, @rjvbb wrote:
  
  > HtH...
  
  
  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.
  
  >> Ideally the strategy when it comes to project file names would be documented somewhere to developers in a comment.
  > 
  > I'm tempted to say that the code should speak for itself, combined with runtime behaviour.
  
  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.
  What I am looking for is some human reader description of the intention. Most perfect when accompanied with unit test data as samples.

INLINE COMMENTS

> openprojectdialog.cpp:77
>      : KAssistantDialog( parent )
> +    , m_projectDirUrl(QUrl())
>      , m_urlIsDirectory(false)

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?
Just remove the line and leave default constructor as applied by compiler do its job.

> openprojectdialog.cpp:315
>  
>  void OpenProjectDialog::validateProjectName( const QString& name )
>  {

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.

> openprojectdialog.cpp:318
> +    if (name != m_projectName) {
> +        bool settingName = currentPage() == projectInfoPage;
> +        m_projectName = name;

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

> openprojectdialog.cpp:332
> +        const QUrl url(m_projectDirUrl);
> +        // construct a version of the project name that's safe for use as a filename:
> +        // TODO: do an additional replace of QDir::separator() with "@"?

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?

> openprojectdialog.cpp:335-337
> +        safeName.replace(QRegularExpression(QStringLiteral("[\\\\/]")), QStringLiteral("@"));
> +        safeName = safeName.replace(QLatin1Char(':'), QLatin1Char('='));
> +        safeName = safeName.replace(QRegExp(QStringLiteral("\\s")), QStringLiteral("_"));

No need to assign to `safeName`, `QString::replace(...)` operates on same object and just returns reference for call chains (see also usage line above).

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/8e65ccae/attachment.html>