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
Branch
fix-cut-paste
Lint
No Linters Available
Unit
No Unit Test Coverage
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?

686

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

712

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

724

join with next line

731

dito

738

dito

823

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

914

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

937

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

956

again, QHash

1060

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

const bool anyFailed = std::any_of(tasks.begin(), tasks.end(),
                                   [](){ ... };
if (anyFailed) {
    ...
}
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).

823

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
78 ↗(On Diff #12349)

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

78 ↗(On Diff #12349)

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

186 ↗(On Diff #12349)

again return a struct, instead of using out parameters

226 ↗(On Diff #12349)

add braces

337 ↗(On Diff #12349)
dialog->setAttribute(Qt::WA_DeleteOnClose);
dialog->show();
plugins/projectmanagerview/cutcopypastehelpers.h
59 ↗(On Diff #12349)

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

63 ↗(On Diff #12349)

please use an enum instead of a boolean here

65 ↗(On Diff #12349)

rename to showWarningDialogForFailedPaste

plugins/projectmanagerview/projectmanagerviewplugin.cpp
1035 ↗(On Diff #11788)

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
78 ↗(On Diff #12349)

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

78 ↗(On Diff #12349)

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

186 ↗(On Diff #12349)

ok

226 ↗(On Diff #12349)

Ok. Please add this to a coding style document.

337 ↗(On Diff #12349)
plugins/projectmanagerview/cutcopypastehelpers.h
59 ↗(On Diff #12349)

ok

63 ↗(On Diff #12349)

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

65 ↗(On Diff #12349)

ok

mwolff added inline comments.Mar 20 2017, 9:09 AM
plugins/projectmanagerview/cutcopypastehelpers.cpp
78 ↗(On Diff #12349)

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.

337 ↗(On Diff #12349)

Yes, because you no longer have a nested eventloop

plugins/projectmanagerview/cutcopypastehelpers.h
63 ↗(On Diff #12349)

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
686

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.