D4772: projectmanagerview: Make cut-paste work by fixing project manager's Paste action

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> projectmanagerviewplugin.cpp:4
>     Copyright 2007 Andreas Pakulat <apaku at gmx.de>
> +   Copyright 2016, 2017 Alexander Potashev <aspotashev at gmail.com>
>  

only 2017, or did you work on this in 2016 already?

> projectmanagerviewplugin.cpp:686
>  
> +struct TaskInfo
> +{

this name is too generic, it's only for copy'n'paste, right? Maybe call it CopyAndPasteTaskInfo? I'd also welcome if you could move this to a separate file, along with the `create*Widget` helpers

> projectmanagerviewplugin.cpp:712
> +    Path m_dest;
> +};
> +

mark this as `Q_DECLARE_TYPEINFO(..., Q_MOVABLE_TYPE);` use QVector instead of QList below

> projectmanagerviewplugin.cpp:724
> +{
> +    return TaskInfo(
> +        ok ? TaskStatus::SUCCESS : TaskStatus::FAILURE,

join with next line

> projectmanagerviewplugin.cpp:731
> +{
> +    return TaskInfo(
> +        ok ? TaskStatus::SUCCESS : TaskStatus::FAILURE,

dito

> projectmanagerviewplugin.cpp:738
> +{
> +    return TaskInfo(
> +        ok ? TaskStatus::SUCCESS : TaskStatus::FAILURE,

dito

> projectmanagerviewplugin.cpp:823
> +{
> +    QDialog* dialog = new QDialog(parent);
> +

I'd welcome if you could move the ui setup stuff into a `.ui` file created with designer

> projectmanagerviewplugin.cpp:914
> +
> +    // For example you are moving the following items into /dest/
> +    //   * /tests/

could you move this stuff into separate helper functions? this looks like multiple steps, and readability would be improved if each step would be its own function

> projectmanagerviewplugin.cpp:937
> +    // file already exists and a replacing copy/move fails.
> +    QMap<Path, Path::List> finalPaths;
> +

does this need to be sorted? if not, use QHash

> projectmanagerviewplugin.cpp:956
> +    // Items originating from projects open in this KDevelop session
> +    QMap<IProject*, QList<KDevelop::ProjectBaseItem*>> itemsPerProject;
> +    // Items that do not belong to known projects

again, QHash

> projectmanagerviewplugin.cpp:1060
> +    // If there was a single failure, display a warning dialog.
> +    if (std::any_of(
> +            tasks.begin(), tasks.end(),

de-inline for readability, join next line, i.e.:

  const bool anyFailed = std::any_of(tasks.begin(), tasks.end(),
                                     [](){ ... };
  if (anyFailed) {
      ...
  }

REPOSITORY
  R33 KDevPlatform

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

To: aspotashev, kfunk, mwolff
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170305/0a54a172/attachment-0001.html>