Add Duplicate feature
AbandonedPublic
Actions

Authored by ngraham on Oct 8 2017, 6:35 PM.

Details

Reviewers

dfaure
elvisangelaccio

Group Reviewers

Dolphin
KDE Applications

Summary

Adds a Duplicate feature to Dolphin, showing up as a menu item in the File menu that appears when one or more items are selected and the directory is writable. Duplicated items receive the names of the original files with " copy" appended before the file extension, if any. If only one item was duplicated, a rename operation is immediately initiated on it, allowing the user to choose a new name.

BUG: 393367
FIXED-IN: 18.08

Test Plan

Tested in KDE Neon.

Test cases:

Try to duplicate when nothing is selected: PASS: menu item is grayed out
Try to duplicate anything on a read-only local volume: PASS: menu item is grayed out
Try to duplicate anything on a read-only samba share: PASS: menu item is grayed out
Duplicate single local file on R/W volume: FAIL: item is duplicated and named correctly, but and a rename operation is not initiated
Duplicate multiple local files on R/W volume: PASS: 3 items are duplicated, named correctly, and selected
Duplicate single local directory on R/W volume: PASS: item is duplicated and named correctly, but a rename operation is not initiated
Duplicate multiple local directories on R/W volume: PASS: 3 items are duplicated, named correctly, and selected
Duplicate single file on R/W samba share: FAIL: item is duplicated and correctly, but a rename operation is not initiated
Duplicate multiple files on R/W samba share: PASS: 3 items are duplicated, named correctly, and selected
Duplicate single directory on R/W samba share: FAIL: item is duplicated and named correctly, but a rename operation is not initiated
Duplicate multiple directory on R/W samba share: PASS: 3 items are duplicated, named correctly, and selected
Try to undo a successful duplication: PASS: operation is undone

The failures are due to an issue where the newly-created items are not added in time to the underlying KFileItemModel. I could use some pointers on fixing this.

This is my first attempt at a big change like this and I'm sure it's full of other issues. I will accept any and all suggestions for improvement. :)

Diff Detail

Repository

R318 Dolphin

Lint

Lint Skipped

Unit

Unit Tests Skipped

ngraham created this revision.Oct 8 2017, 6:35 PM

ngraham edited the test plan for this revision. (Show Details)Oct 8 2017, 6:39 PM

Didn't do this right. Correct revision: https://phabricator.kde.org/D8208

Re-opening to restart the discussion since apparently now it's not just me; we got a user request for this feature: https://bugs.kde.org/show_bug.cgi?id=393367

Is there anything that could make folks accept this?

fazevedo added a subscriber: fazevedo.Apr 22 2018, 11:30 AM

fazevedo added inline comments.

src/views/dolphinview.cpp
724	const auto &item
736	.clear()
745	I would unconditionally append the " copy", adding an incremented number until the final file path does not exists yet.
771	This is entirely wrong because the copy is asynchronous. You most likely need to keep a temp list and check it when a job finished to see if it should be selected or not. Best is probably to check what is doing the copy selected items code path as it does exactly what you are trying to do.
774	This feel very wrong because each time one initiate a duplicate operation, you will connect again and again the same sender/receiver. And I'm not even sure you would need that ?
777	This feel wrong because if it's a folder you could rename a folder while its content is not yet finished copying. So I suggest you remove it, or explicitly check this is a file (or symlink to a file)
src/views/dolphinviewactionhandler.cpp
136	I would put the duplicate action in the group of copy/cut/paste.

By the way, you can abandon again this one, the correct review is https://phabricator.kde.org/D8208

Oops, re-opened the wrong revision.

markg added a subscriber: markg.Apr 22 2018, 3:23 PM

markg added inline comments.

src/views/dolphinviewactionhandler.cpp
134	No.. Not another app with another binding for CTRL+D.. :( In Chrome CTRL+D is bookmark. In Konsole CTRL+D is exits the current session .. I'd prefer to just not have a key binding for it by default. The users can - if they want - add a binding themselves in the shortcut settings. Not sure if you need to do something for it to appear in that settings page though.

Sorry for the confusion. Let's take all of our comments to D8208.

Revision Contents
Changeset List

			Path	Packages
M			src/dolphinmainwindow.cpp (2 lines)
M			src/dolphinui.rc (7 lines)
M			src/views/dolphinview.h (7 lines)
M			src/views/dolphinview.cpp (74 lines)
M			src/views/dolphinviewactionhandler.h (5 lines)
M			src/views/dolphinviewactionhandler.cpp (13 lines)