projectmanagerview: Add Cut command into the context menu
ClosedPublic

Authored by aspotashev on Jun 18 2016, 10:03 PM.

Details

Test Plan
  1. "Cut" in KDevelop, "Paste" into Dolphin - should move the file,
  2. "Copy" in KDevelop, "Paste" into Dolphin - should copy the file.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aspotashev retitled this revision from to projectmanagerview: Add Cut command into the context menu.Jun 18 2016, 10:03 PM
aspotashev updated this object.
aspotashev edited the test plan for this revision. (Show Details)
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 18 2016, 10:03 PM
kfunk requested changes to this revision.Jun 18 2016, 10:30 PM
kfunk added a reviewer: kfunk.
kfunk added a subscriber: kfunk.

Please do it in the order: Cut/Copy/Paste and add separators.

This revision now requires changes to proceed.Jun 18 2016, 10:30 PM
aspotashev updated this revision to Diff 4755.Jun 26 2016, 10:38 PM
  • ProjectManagerViewPlugin: Rename needsRemoveAndRename to needsCutRenameRemove
  • ProjectManagerViewPlugin: Reorder and group file actions cut/copy/paste and rename/remove
kfunk requested changes to this revision.Jul 13 2016, 12:26 PM

Just a quick code inspection; I'll try the feature as soon as I find time.

plugins/projectmanagerview/projectmanagerviewplugin.cpp
683 ↗(On Diff #4755)

Could do auto ctx = ...

687 ↗(On Diff #4755)

Not needed

692 ↗(On Diff #4755)

KFileItem(url).mostLocalUrl() should work?

754 ↗(On Diff #4755)

Can be a free function inside namespace {}.

I'd also rename to createSeparatorAction

This revision now requires changes to proceed.Jul 13 2016, 12:26 PM
aspotashev updated this revision to Diff 6429.Sep 4 2016, 7:53 PM

Addressed all comments by Kevin.

aspotashev marked 3 inline comments as done.Sep 4 2016, 7:54 PM
aspotashev added inline comments.
plugins/projectmanagerview/projectmanagerviewplugin.cpp
754 ↗(On Diff #4755)

Changed to a static function instead because namespace {} takes more lines of code and is harder to read.

mwolff accepted this revision.Sep 5 2016, 3:04 PM
mwolff added a reviewer: mwolff.
mwolff added a subscriber: mwolff.

seems to be ok, thanks!

kfunk requested changes to this revision.Sep 6 2016, 10:06 AM

Just tried this locally.

The "Cut" action copies the file for me -- can you verify it is working as intended? I don't see anything wrong with your code, though.

Relevant debug output:

[kdevelop(15865)/(kdevplatform.plugins.projectmanagerview) ProjectManagerViewPlugin::createClipboardMimeData(701): (QUrl("file:///home/kfunk/devel/src/icemon/cmake"))
[kdevelop(15865)/(kdevplatform.plugins.projectmanagerview) ProjectBuildSetWidget::selectionChanged(100): checking selectionmodel: ()
[kdevelop(15865)/(kdevplatform.vcs) KDevelop::DVcsJob::start(183): Execute dvcs command: "git stash list"
[kdevelop(15865)/(kdevplatform.vcs) KDevelop::DVcsJob::start(183): Execute dvcs command: "git stash list"
[kdevelop(15865)/(kdevplatform.plugins.projectmanagerview) ProjectManagerViewPlugin::pasteFromContextMenu(734): (QUrl("file:///home/kfunk/devel/src/icemon/cmake"))
[kdevelop(15865)/(kf5.kcoreaddons.kdirwatch) KDirWatchPrivate::stopEntryScan(1146): "KDirWatch-7" stopped scanning "/home/kfunk/devel/src/icemon/doc" (now 0 watchers)
[kdevelop(15865)/(kf5.kio.core.copyjob) KIO::copy(2138): src= QUrl("file:///home/kfunk/devel/src/icemon/cmake") dest= QUrl("file:///home/kfunk/devel/src/icemon/doc")
[kdevelop(15865)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotStart(334): CopyJob: stating the dest QUrl("file:///home/kfunk/devel/src/icemon/doc")
...
This revision now requires changes to proceed.Sep 6 2016, 10:06 AM

Kevin,

Cut is fully functional, but Paste isn't. The following steps do work:

  1. Cut in KDevelop
  2. Paste in Dolphin.
kfunk added a comment.Sep 7 2016, 7:29 AM

Kevin,

Cut is fully functional, but Paste isn't. The following steps do work:

  1. Cut in KDevelop
  2. Paste in Dolphin.

Yeah, I did cut in KDevelop and then paste in KDevelop.

That needs to be working; otherwise it's totally confusing for end users. Cut+Paste *inside* KDevelop would be the main use-case; pasting into Dolphin is just secondary.

mwolff requested changes to this revision.Feb 18 2017, 10:44 PM

this is a really useful feature, I would love to see it fully implemented (i.e. also with paste). Can you do that? Or should we take over?

plugins/projectmanagerview/projectmanagerviewplugin.cpp
64 ↗(On Diff #6429)

instead of static, wrap it in an anonymous namespace

the result is the same, but I much prefer the C++ way of doing this

Milian,

I've started coding the proper paste algorithm last night, seems to be doable. Expect another patch this week, please ping me otherwise.

A cut-honoring Paste is implemented in https://phabricator.kde.org/D4772. After that one is merged, this patch may be fixed and merged as well to receive a Cut command in KDevelop.

aspotashev updated this revision to Diff 14516.May 14 2017, 6:56 PM
  • Rebase on top of master
  • Move createClipboardMimeData() outside of class ProjectManagerViewPlugin
  • Move static functions into anonymous namespace
aspotashev updated this revision to Diff 14517.May 14 2017, 7:10 PM
  • Replace foreach with range for
aspotashev added inline comments.May 15 2017, 1:22 PM
plugins/projectmanagerview/projectmanagerviewplugin.h
35 ↗(On Diff #14517)

Oops, this is not needed anymore.

aspotashev updated this revision to Diff 14572.May 15 2017, 9:23 PM
  • Remove forward declaration of class QMimeData
mwolff added inline comments.May 24 2017, 1:12 PM
plugins/projectmanagerview/projectmanagerviewplugin.cpp
278 ↗(On Diff #14572)

this means we cannot paste into the project root, no? since the project root has no parent, but is a folder?

aspotashev added inline comments.May 25 2017, 5:11 PM
plugins/projectmanagerview/projectmanagerviewplugin.cpp
278 ↗(On Diff #14572)

No, this means we cannot cut/rename/delete the project root. I didn't invent this logic, it has been there and worked well for a few years already.

mwolff accepted this revision.Sep 1 2017, 10:48 AM

sorry for the long delay, has this not been merged yet? I'll try to do this now

This revision was automatically updated to reflect the committed changes.