Allow to copy or move selection to the other split view
ClosedPublic

Authored by aprcela on Apr 20 2020, 9:47 AM.

Details

Summary

FEATURE: 276167

Default keyboard shortcuts set to SHIFT+F5 for copy, SHIFT+F6 for move

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
dfaure accepted this revision.May 3 2020, 8:08 AM

Looks good to me.

Did you test copying and moving directories as well? Should work, but just to make sure.

I'll let the Dolphin developers/maintainers decide on whether the shortcuts are OK, and land this.

This revision is now accepted and ready to land.May 3 2020, 8:08 AM

Looks good to me.

Did you test copying and moving directories as well? Should work, but just to make sure.

I'll let the Dolphin developers/maintainers decide on whether the shortcuts are OK, and land this.

Yes, tested:
copy/move file & multiple files
copy/move folder & multiple folder
copy/move both folder and file & both multiple folders and multiple files

Shall we keep the keyboard shortcuts as CTRL+F5 for copy and CTRL+F6 for move. Krusader uses F5 for copy and F6 for move. So it would be almost same (F5 is refresh in dolphin).

https://kde.org/applications/utilities/org.kde.krusader

meven requested changes to this revision.May 3 2020, 10:35 AM

I tested the patch, works nicely.
Just a check to add.

Drag'n drop is quite easy in the use case it covers, so wait for @ngraham and/or @elvisangelaccio feedback before merging.

src/dolphinmainwindow.cpp
1960

This is not sufficient, you can copy or move to recentlyused:/files/ for instance, which fails.
So test the other view url to see if it is writable as well.

This revision now requires changes to proceed.May 3 2020, 10:35 AM
dfaure requested changes to this revision.May 3 2020, 11:28 AM

Actually, wait, I vote against Ctrl+F5/Ctrl+F6 because these shortcuts, by default, in Plasma, are bound to "Switch to Desktop 5" and "Switch to Desktop 6".

Shift+F5/F6 would be much better, if it's available.

Meven: drag-n-drop is 'easy' but it requires using the mouse. For accessibility, or for people stuck on a plane with a bad touchpad and no room for a real mouse, or simply for maximum performance in optimized workflows, I can see the benefit of keyboard shortcuts.

Just not if they switch desktops inadvertently, once you configure 6+ virtual desktops :-)

Just a check to add.

@meven See inline comment

Actually, wait, I vote against Ctrl+F5/Ctrl+F6 because these shortcuts, by default, in Plasma, are bound to "Switch to Desktop 5" and "Switch to Desktop 6".

Shift+F5/F6 would be much better, if it's available.

Meven: drag-n-drop is 'easy' but it requires using the mouse. For accessibility, or for people stuck on a plane with a bad touchpad and no room for a real mouse, or simply for maximum performance in optimized workflows, I can see the benefit of keyboard shortcuts.

Just not if they switch desktops inadvertently, once you configure 6+ virtual desktops :-)

Shift+F5/F6 seems to bee free. So we could go with that.

src/dolphinmainwindow.cpp
1960

I'm trying to add a check and it only doesn't work only on this kind of folders. Works fine with local and samba share. See diff:

diff --git a/src/dolphinmainwindow.cpp b/src/dolphinmainwindow.cpp
index 428c61952..5e7c56385 100644
--- a/src/dolphinmainwindow.cpp
+++ b/src/dolphinmainwindow.cpp
@@ -1959,7 +1959,7 @@ void DolphinMainWindow::updateFileAndEditActions()
 {
     const KFileItemList list = m_activeViewContainer->view()->selectedItems();
     const KActionCollection* col = actionCollection();
-    KFileItemListProperties capabilities(list);
+    KFileItemListProperties capabilitiesSource(list);
 
     QAction* addToPlacesAction = col->action(QStringLiteral("add_to_places"));
     QAction* copyToOtherViewAction   = col->action(QStringLiteral("copy_to_other_split_view"));
@@ -1989,22 +1989,31 @@ void DolphinMainWindow::updateFileAndEditActions()
         }
 
         if (m_tabWidget->currentTabPage()->splitViewEnabled()) {
-            copyToOtherViewAction->setEnabled(true);
-            moveToOtherViewAction->setEnabled(capabilities.supportsMoving());
+            DolphinTabPage* tabPage = m_tabWidget->currentTabPage();
+            KFileItem capabilitiesDestination;
+
+            if (tabPage->primaryViewActive()) {
+                capabilitiesDestination = tabPage->secondaryViewContainer()->url();
+            } else {
+                capabilitiesDestination = tabPage->primaryViewContainer()->url();
+            }
+
+            copyToOtherViewAction->setEnabled(capabilitiesDestination.isWritable());
+            moveToOtherViewAction->setEnabled(capabilitiesSource.supportsMoving() && capabilitiesDestination.isWritable());
         } else {
             copyToOtherViewAction->setEnabled(false);
             moveToOtherViewAction->setEnabled(false);
         }
 
-        const bool enableMoveToTrash = capabilities.isLocal() && capabilities.supportsMoving();
+        const bool enableMoveToTrash = capabilitiesSource.isLocal() && capabilitiesSource.supportsMoving();
 
-        renameAction->setEnabled(capabilities.supportsMoving());
+        renameAction->setEnabled(capabilitiesSource.supportsMoving());
         moveToTrashAction->setEnabled(enableMoveToTrash);
-        deleteAction->setEnabled(capabilities.supportsDeleting());
-        deleteWithTrashShortcut->setEnabled(capabilities.supportsDeleting() && !enableMoveToTrash);
-        cutAction->setEnabled(capabilities.supportsMoving());
+        deleteAction->setEnabled(capabilitiesSource.supportsDeleting());
+        deleteWithTrashShortcut->setEnabled(capabilitiesSource.supportsDeleting() && !enableMoveToTrash);
+        cutAction->setEnabled(capabilitiesSource.supportsMoving());
         showTarget->setEnabled(list.length() == 1 && list.at(0).isLink());
-        duplicateAction->setEnabled(capabilities.supportsWriting());
+        duplicateAction->setEnabled(capabilitiesSource.supportsWriting());
     }
 }
elvisangelaccio requested changes to this revision.May 3 2020, 5:41 PM

The feature looks reasonable to me.

src/dolphinmainwindow.cpp
670–684

Can you try to add a DolphinTabWidget::copyToOtherView() function and move this logic there?

686–700

Can you try to add a DolphinTabWidget::moveToOtherView() function and move this logic there?

src/views/dolphinview.cpp
1229

I see.

@aprcela Can you add this information in the commit message? Otherwise it will get lost.

src/views/dolphinview.h
373–383

Nitpick: DolphinView doesn't know what a "split view" is. I'd rather call these methods copySelectedItems() and moveSelectedItems(), since that's what they are actually doing.

aprcela updated this revision to Diff 81826.May 3 2020, 8:16 PM
aprcela marked 2 inline comments as done.
  • Add check if destination is writable for copy
  • Move copy/move logic to DolphinTabWidget
  • Set Shift+F5/F6 instead of CTRL+F5/F6 as keyboard shortcut
aprcela marked 2 inline comments as done.EditedMay 3 2020, 8:23 PM

@elvisangelaccio
copySelectedItems() is already used to copy to clipboard. See L365.

@meven
I uploaded the currently working code (to check if destination is writable for the copy function). But I can't get it to work for the location you provided: recentlyused:/files/

src/views/dolphinview.cpp
1229

It's in the commit 5359352b0ef1. Is that enough?

src/views/dolphinview.h
373–383

copySelectedItems() is already used to copy to clipboard. See L365.

aprcela marked 2 inline comments as done.May 3 2020, 8:25 PM
meven added inline comments.May 4 2020, 8:03 AM
src/views/dolphinview.cpp
1229

No your individual commits are supposed to get squashed upon merging.

aprcela marked an inline comment as done.May 4 2020, 9:15 AM
aprcela added inline comments.
src/views/dolphinview.cpp
1229

Ok. I'm gonna make a separate patch via phabricator.

aprcela updated this revision to Diff 81850.May 4 2020, 9:16 AM
aprcela marked an inline comment as done.
  • Revert "Instead of append, change the value to a simplifiedUrlList"
aprcela marked an inline comment as done.May 4 2020, 9:23 AM
aprcela added inline comments.
src/views/dolphinview.cpp
1229
aprcela marked an inline comment as done.May 4 2020, 9:23 AM
aprcela marked an inline comment as done.
aprcela updated this revision to Diff 81854.May 4 2020, 9:33 AM
aprcela edited the summary of this revision. (Show Details)May 4 2020, 10:22 AM
aprcela edited the summary of this revision. (Show Details)
aprcela updated this revision to Diff 81872.May 4 2020, 11:07 AM
  • Rewrite so it's obvious that it is copying/moving to the inactive view
aprcela added a comment.EditedMay 7 2020, 8:16 AM

@meven
These seem to be 'special-cases' locations.
The call to capabilitiesDestination.isWritable() :

Does not work properly for these. It returns true, but the locations aren't writable. An error message pops up when trying to move/copy here):
remote:/
recentlyused:/
search:/

Does work as expected, following destination is writable and return value of .isWritable() is true:
trash:/

Can't test:
/media/nfs
/media/floppy0
/media/XO-Y4
/media/cdrom

What would be a good approach to solve this?

It sounds to me like you found bugs in some kioslaves. Fixing that is out of scope for this change request. Certainly a good idea to do that, but IMHO not a blocker for this to go in.

dfaure accepted this revision.May 10 2020, 3:39 PM

Good from a KIO point of view, but this needs approval from Dolphin people.

meven added a comment.May 10 2020, 5:21 PM

It sounds to me like you found bugs in some kioslaves. Fixing that is out of scope for this change request. Certainly a good idea to do that, but IMHO not a blocker for this to go in.

I don't think this is a bug in any ioslave : dolphin view doesn't have this issue when copy/pasting files in , and for instance recentlyused:/ has "writing=false" and "moving=false" in its json file.
So I would guess this is not a bug there but somewhere else along the call chain. One can compare this code to DolphinContextMenu::createPasteAction that uses KIO::pasteActionText

I agree this should not keep this bug from landing.
But it would be great to have a look as to where the issue comes from...

One approval from @ngraham or @elvisangelaccio and we should land this.

aprcela added a comment.EditedMay 10 2020, 5:34 PM

It sounds to me like you found bugs in some kioslaves. Fixing that is out of scope for this change request. Certainly a good idea to do that, but IMHO not a blocker for this to go in.

I don't think this is a bug in any ioslave : dolphin view doesn't have this issue when copy/pasting files in , and for instance recentlyused:/ has "writing=false" and "moving=false" in its json file.
So I would guess this is not a bug there but somewhere else along the call chain. One can compare this code to DolphinContextMenu::createPasteAction that uses KIO::pasteActionText

I agree this should not keep this bug from landing.

erm, I don't want to land a Bug :D

I'm gonna check this one again later on to see if I can find anything.

So, after comparing to DolphinContextMenu::createPasteAction , the function used for my copy/move is KIO::move & KIO::copy whereas createPasteAction uses KIO::pasteActionText which has mimeData in it. This mimeData requires that there's some data in the clipboard, KIO::move/copy don't use the clipboard.

What I also noticed, the following error messages pop up in konsole when i switch to a problematic folder like recentlyused:/files: kf5.kio.core: Invalid URL: QUrl("")

@meven where can i find the .json for recentlyused:/ ? Nvm, found it

src/views/dolphinview.h
373–383

Not anymore since d1a70c0b62 ;)

aprcela updated this revision to Diff 82609.May 11 2020, 9:12 PM
aprcela marked an inline comment as done.May 11 2020, 9:13 PM
aprcela added inline comments.
src/views/dolphinview.h
373–383

Thanks. I renamed those now :)

src/views/dolphinview.h
377

Let's just call it destinationUrl. This can be any URL, not necessarily the URL of a split pane.

383

Same here.

aprcela updated this revision to Diff 82628.May 12 2020, 7:14 AM
aprcela marked an inline comment as done.
  • Renamed to destinationUrl
aprcela marked 3 inline comments as done.May 12 2020, 7:15 AM
aprcela added inline comments.
src/views/dolphinview.h
377

Yeah, makes sense. Done

aprcela marked 2 inline comments as done.May 12 2020, 7:15 AM
elvisangelaccio accepted this revision.May 12 2020, 8:05 PM
meven accepted this revision.May 14 2020, 7:00 AM

Rather than merging master into your branches, I would recommend you rebasing ;)

This revision is now accepted and ready to land.May 14 2020, 7:00 AM

Rather than merging master into your branches, I would recommend you rebasing ;)

Ok, thanks for the information! :)

Nice feature. I would rename the menu items a bit:
"Move to inactive split view"
"Copy to inactive split view"

No need to mention "the selection" because it's implicit. And I would mention that this is about the split view, or else when not using the split view feature, it's not clear how to enable the menu items. If you mention splits in the name though, it's advertising the split view feature for whose who haven't found it yet.

aprcela updated this revision to Diff 82880.May 14 2020, 9:19 PM
  • Minor renaming of text and the according variables
  • Merge remote-tracking branch 'origin' into arcpatch-D29006

Nice feature. I would rename the menu items a bit:
"Move to inactive split view"
"Copy to inactive split view"

No need to mention "the selection" because it's implicit. And I would mention that this is about the split view, or else when not using the split view feature, it's not clear how to enable the menu items. If you mention splits in the name though, it's advertising the split view feature for whose who haven't found it yet.

Did the renaming. And sorry @meven just had a hard time rebasing now.. :/

ngraham accepted this revision.May 14 2020, 9:46 PM

Thanks!

ngraham requested changes to this revision.May 14 2020, 9:50 PM

Actually sorry, I have a few more comments before I think this can land:

src/dolphintabwidget.h
196

"the other view" is not very informative. How about {move,copy}ToInactiveSplitView(), to mirror the name of the action itself?

src/dolphinui.rc
2

bump to 31, not 32

src/views/dolphinview.h
377

ditto for the names here

This revision now requires changes to proceed.May 14 2020, 9:50 PM
aprcela updated this revision to Diff 82883.May 14 2020, 10:14 PM
aprcela marked 2 inline comments as done.
  • Renaming of functions
aprcela marked an inline comment as done.May 14 2020, 10:15 PM

Done :)

ngraham accepted this revision.May 14 2020, 10:19 PM

Thank you!

This revision is now accepted and ready to land.May 14 2020, 10:19 PM
This revision was automatically updated to reflect the committed changes.