D8206: Add Duplicate feature

[Filipe Azevedo]

fazevedo added inline comments.

INLINE COMMENTS

> dolphinview.cpp:724
> +    // If multiple items are selected, duplicate them all
> +    for (KFileItem item : itemList) {
> +        originalURL  = item.url();

const auto &item

> dolphinview.cpp:736
> +        if (suffix.isEmpty()) {
> +            suffixString = "";
> +        } else {

.clear()

> dolphinview.cpp:745
> +            duplicatePath = originalPath + QString(i18n(" copy")) + suffixString;
> +        }
> +        duplicateURL.setPath(duplicatePath);

I would unconditionally append the " copy", adding an incremented number until the final file path does not exists yet.

> dolphinview.cpp:771
> +    selectionManager->setSelectedItems(duplicatedItems);
> +    emitSelectionChangedSignal();
> +

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.

> dolphinview.cpp:774
> +    connect(m_view, &DolphinItemListView::roleEditingFinished,
> +                this, &DolphinView::slotRoleEditingFinished);
> +

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 ?

> dolphinview.cpp:777
> +    if (m_selectedUrls.count() == 1) {
> +        renameSelectedItems();
> +    }

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)

> dolphinviewactionhandler.cpp:136
> +    duplicateAction->setEnabled(false);
> +    connect(duplicateAction, &QAction::triggered, this, &DolphinViewActionHandler::slotDuplicate);
> +

I would put the duplicate action in the group of copy/cut/paste.

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D8206

To: ngraham, #dolphin, #kde_applications, dfaure, elvisangelaccio
Cc: fazevedo, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180422/c8c40493/attachment.htm>