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

Authored by aspotashev on Feb 24 2017, 7:13 PM.

Details

Test Plan

1a. Copy-paste from a Git-based project into the same project.
1b. Same, but cut-paste.

  1. Copy/cut-paste from a Git-based project into another open project.
  2. Copy/cut-paste from a place not tracked by KDevelop (use Dolphin to copy/cut).
  3. Copy/cut-paste into a folder in KDevelop without write permissions -> expected to display a warning dialog "Paste Failed".
  4. Select a folder [A] and its subitem [B]; cut/copy-paste it -> expected to copy the enclosing folder and select both [A] and [B] in the project manager view.
  5. Create a folder [A] without write permissions. Choose an item [B] from outside of [A]. Create an item [C] named like [B] inside of [A] (you probably have to "sudo" to gain write permissions). Copy/cut-paste [B] into [A] attempting to overwrite [C]. Expect a warning dialog "Paste Failed". Expect [C] to not be selected in the project manager view (as it hasn't been successfully written to).

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aspotashev created this revision.Feb 24 2017, 7:13 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptFeb 24 2017, 7:13 PM

Screenshots of the warning dialog:

mwolff requested changes to this revision.Mar 5 2017, 10:05 PM
mwolff added inline comments.
plugins/projectmanagerview/projectmanagerviewplugin.cpp
4

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

678

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

704

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

713

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

716

join with next line

723

dito

730

dito

736

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

755

again, QHash

781

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

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

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

This revision now requires changes to proceed.Mar 5 2017, 10:05 PM
aspotashev updated this revision to Diff 12349.Mar 10 2017, 6:36 AM
aspotashev edited edge metadata.
aspotashev marked an inline comment as done.
  • Addressed all comments except for creation of a .ui file
aspotashev added inline comments.Mar 10 2017, 6:46 AM
plugins/projectmanagerview/projectmanagerviewplugin.cpp
4

Changed to 2017. I worked on Cut in 2016 (the other patch).

815

I attempted to implement the UI setup as a .ui file, but decided that it won't be clean enough, for example:

  1. I couldn't insert an icon into .ui as it requires a QPixmap, while QIcon::fromTheme returns QIcon and that requires run-time conversion to a pixmap.
  2. Couldn't find out how to replace iconLayout->addStretch(1) in .ui: vertical spacers have no property "stretch" in Qt Designer.

Btw, the UI code is based on implementation of KF5's KMessageBox.

mwolff requested changes to this revision.Mar 19 2017, 1:15 PM

sorry for the delay Alexander

plugins/projectmanagerview/cutcopypastehelpers.cpp
79

add a TODO to create a model for the task list, and use it here instead of using QTreeWidget

79

also here and elsewhere: do not use foreach, use range-based for

187

again return a struct, instead of using out parameters

227

add braces

338
dialog->setAttribute(Qt::WA_DeleteOnClose);
dialog->show();
plugins/projectmanagerview/cutcopypastehelpers.h
60

rename to mapSourceToDestination, return a struct with the mapping instead of taking two out-parameters

rename first paths argument to sourcePaths, rename destPath to destinationPath

64

please use an enum instead of a boolean here

66

rename to showWarningDialogForFailedPaste

plugins/projectmanagerview/projectmanagerviewplugin.cpp
745

add braces

This revision now requires changes to proceed.Mar 19 2017, 1:15 PM
aspotashev requested review of this revision.Mar 19 2017, 8:39 PM
aspotashev edited edge metadata.
aspotashev added inline comments.
plugins/projectmanagerview/cutcopypastehelpers.cpp
79

Ok. But why using a model? QTreeWidget is enough for the current feature-poor widget.

79

Ok. I couldn't find KDevelop coding style on the net, is it available?

187

ok

227

Ok. Please add this to a coding style document.

338
plugins/projectmanagerview/cutcopypastehelpers.h
60

ok

64

Why? There are no other forthcoming options besides Cut and Copy. At least KIO::isClipboardDataCut() returns a boolean.

66

ok

mwolff added inline comments.Mar 20 2017, 9:09 AM
plugins/projectmanagerview/cutcopypastehelpers.cpp
79

We don't really have such a document, we should start one... But on foreach see: https://www.kdab.com/goodbye-q_foreach/

Regarding models vs. QTreeWidget: A model is in general the cleaner solution, as it better separates the data from the presentation. I'm not demanding you to implement the model directly, just that you add a TODO that this should eventually be cleaned up.

338

Yes, because you no longer have a nested eventloop

plugins/projectmanagerview/cutcopypastehelpers.h
64

stylistic nitpick, it makes the code easier to read.

copyMoveItems(..., true);
copyMoveItems(..., false);

Assume you read this in two years, do you know what the booleans are doing? Compare that to:

copyMoveItems(..., CutCopyPasteHelpers::Cut);
copyMoveItems(..., CutCopyPasteHelpers::Copy);
mwolff requested changes to this revision.Mar 22 2017, 8:59 AM
This revision now requires changes to proceed.Mar 22 2017, 8:59 AM
aspotashev updated this revision to Diff 14018.Apr 30 2017, 9:45 AM
aspotashev edited edge metadata.
aspotashev marked 29 inline comments as done.
  • Addressed nearly all change requests.
plugins/projectmanagerview/projectmanagerviewplugin.cpp
678

This method is as generic as "ProjectManagerView::selectItems()", can be used beyond copy-cut-paste.

mwolff accepted this revision.May 14 2017, 10:15 AM

Sorry for the delay, and thanks for the contribution. You do have commit rights, correct? If so, please push to master.

Thanks again!

This revision is now accepted and ready to land.May 14 2017, 10:15 AM
This revision was automatically updated to reflect the committed changes.