FEATURE: 276167
Default keyboard shortcuts set to SHIFT+F5 for copy, SHIFT+F6 for move
elvisangelaccio | |
ngraham | |
meven | |
dfaure |
Dolphin |
FEATURE: 276167
Default keyboard shortcuts set to SHIFT+F5 for copy, SHIFT+F6 for move
Automatic diff as part of commit; lint not applicable. |
Automatic diff as part of commit; unit tests not applicable. |
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).
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. |
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 :-)
@meven See inline comment
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()); } } |
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. |
@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. |
src/views/dolphinview.cpp | ||
---|---|---|
1229 | No your individual commits are supposed to get squashed upon merging. |
src/views/dolphinview.cpp | ||
---|---|---|
1229 | Ok. I'm gonna make a separate patch via phabricator. |
src/views/dolphinview.cpp | ||
---|---|---|
1229 |
@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.
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.
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 ;) |
src/views/dolphinview.h | ||
---|---|---|
373–383 | Thanks. I renamed those now :) |
src/views/dolphinview.h | ||
---|---|---|
377 | Yeah, makes sense. Done |
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.
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 |