Add Duplicate feature
ClosedPublic

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

Details

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.

Test Plan

  1. Delete or temporarily move aside ~/.local/share/kxmlgui5/dolphin/dolphinui.rc
  2. Launch Dolphin
  3. Select one item
  4. Ctrl+D or file > Duplicate Here or > right-click > Duplicate Here
  5. Try the above with multiple files selected
  6. Do it all again with different combinations of single and multiple files with various filename extensions

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
cfeck added a comment.Jan 10 2020, 9:23 AM

Of course the message should have the usual "Don't show again" checkbox.

pino added a subscriber: pino.Jan 10 2020, 9:43 AM
pino added inline comments.
src/views/dolphinview.cpp
720

using QFileInfo will break if the url is not a local file

I think I could live without a warning message, if the item is named 'Duplicate here' (or 'Duplicate Here'?) to clarify that you shouldn't use it to get a duplicate elsewhere.

In D8208#591262, @cfeck wrote:

I think I could live without a warning message, if the item is named 'Duplicate here' (or 'Duplicate Here'?) to clarify that you shouldn't use it to get a duplicate elsewhere.

Sure, I can do that. Makes sense.

ngraham planned changes to this revision.Jan 10 2020, 1:39 PM
ngraham marked an inline comment as done.Jan 10 2020, 7:29 PM
ngraham updated this revision to Diff 73232.Jan 10 2020, 7:33 PM
  • Don't use QFileInfo
  • Improve reliability of "copy" text appending with unusual filenames
  • Add the action to the context menu
fvogt requested changes to this revision.Jan 10 2020, 9:07 PM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
src/views/dolphinview.cpp
723

Note that this returns a null QString if the extension isn't known and also doesn't preserve case.

So currently this would happen:

"test.foo" -> "test.foo"/ ""` -> "test.foo copy"
"test.GIF" -> "test"/ "gif"` -> "test.GIF copy.gif"

This revision now requires changes to proceed.Jan 10 2020, 9:07 PM

@fvogt "test.foo" becoming "test.foo copy" is intentional because .foo isn't a known valid extension, and it's valid for files to have dots in their names. In general the approach of using QMimeDatabase supports this, solves the problems of using QFileInfo that @pino pointed out, and also allows seamless support for filename extensions with two dots in them (e.g. tar.gz).

If I go back to using QFileInfo to chop up the file into filename + extension, then there's the difficult choice of what kind of file to fail on: If I use QFileInfo::suffix(), it will fail for extensions with two dots like .tar.gz; if I use QFileInfo::completeSuffix(), that fails for files with dots in their names outside of the extension.

Each approach has drawbacks. Which would you find acceptable?

@fvogt "test.foo" becoming "test.foo copy" is intentional because .foo isn't a known valid extension, and it's valid for files to have dots in their names. In general the approach of using QMimeDatabase supports this, solves the problems of using QFileInfo that @pino pointed out, and also allows seamless support for filename extensions with two dots in them (e.g. tar.gz).

If I go back to using QFileInfo to chop up the file into filename + extension, then there's the difficult choice of what kind of file to fail on: If I use QFileInfo::suffix(), it will fail for extensions with two dots like .tar.gz; if I use QFileInfo::completeSuffix(), that fails for files with dots in their names outside of the extension.

Each approach has drawbacks. Which would you find acceptable?

The case preserving issue is more severe than the issue with unknown extensions, so if that's fixed, using QMimeDatabase is IMO fine. Maybe with a fallback to completeSuffix if it didn't have an entry.

fazevedo added a comment.EditedJan 11 2020, 1:45 PM

@fvogt "test.foo" becoming "test.foo copy" is intentional because .foo isn't a known valid extension, and it's valid for files to have dots in their names. In general the approach of using QMimeDatabase supports this, solves the problems of using QFileInfo that @pino pointed out, and also allows seamless support for filename extensions with two dots in them (e.g. tar.gz).

If I go back to using QFileInfo to chop up the file into filename + extension, then there's the difficult choice of what kind of file to fail on: If I use QFileInfo::suffix(), it will fail for extensions with two dots like .tar.gz; if I use QFileInfo::completeSuffix(), that fails for files with dots in their names outside of the extension.

Each approach has drawbacks. Which would you find acceptable?

Why trying to append 'copy' in the base name whith possible headaches while you could simply do something like 'Copy N#1 of /original file name/' ?
The only thing to test would be if the filename starts with a dot and and keep it as first.

fvogt added a comment.Jan 11 2020, 1:47 PM

@fvogt "test.foo" becoming "test.foo copy" is intentional because .foo isn't a known valid extension, and it's valid for files to have dots in their names. In general the approach of using QMimeDatabase supports this, solves the problems of using QFileInfo that @pino pointed out, and also allows seamless support for filename extensions with two dots in them (e.g. tar.gz).

If I go back to using QFileInfo to chop up the file into filename + extension, then there's the difficult choice of what kind of file to fail on: If I use QFileInfo::suffix(), it will fail for extensions with two dots like .tar.gz; if I use QFileInfo::completeSuffix(), that fails for files with dots in their names outside of the extension.

Each approach has drawbacks. Which would you find acceptable?

Why trying to append 'copy' in the base name whith possible headaches while you could simply do something like 'Copy N#1 of /original file name/' ?

That's what windows (xp, no idea about newer) does as well.

pino removed a subscriber: pino.Jan 11 2020, 1:48 PM
ngraham marked 2 inline comments as done.Jan 11 2020, 6:00 PM

I can do that, sure. But IMO there's a UX problem when you append "Copy of" to the beginning rather than the end. Imagine that you have a file that starts with a Z and you duplicate it. The duplicate appears near the top of the window because it begins with C. If the current view is scrollable, view scrolls up to the top to show you the newly duplicated file. This causes you to lose your place in the view, which could be annoying if you were planning to do something else with another adjacent file or perform another operation on the original file (perhaps to duplicate it a second time to perform some A/B testing on two duplicates with slightly different changes, for example). For this reason, I prefer the macOS behavior of appending "copy" to the end of the filename, not to the beginning.

ngraham planned changes to this revision.Jan 11 2020, 6:13 PM

Will fix bugs in text appending behavior

How about I fix suffixForFileName() to preserve case?

That would be nice too. :)

ngraham updated this revision to Diff 73291.Jan 11 2020, 7:10 PM
  • Fix bug with copy being put in the wrong place for filenames with extensions
  • Preserve existing filename extension casing
ngraham retitled this revision from [WIP] Add Duplicate feature to Add Duplicate feature.Jan 11 2020, 7:10 PM
ngraham edited the test plan for this revision. (Show Details)
fvogt resigned from this revision.Jan 11 2020, 7:12 PM

Hmm I don't think I should change the behaviour of QMimeDatabase after all.

https://lxr.kde.org/source/kde/kdeutils/ark/kerfuffle/createdialog.cpp#0106 does

const QString detectedSuffix = QMimeDatabase().suffixForFileName(fileName);
if (currentMimeType().suffixes().contains(detectedSuffix)) {

which would break if case was preserved.

fileName.right(extension.size()) is exactly the way I almost-fixed this in Qt, so it's the right thing to do here instead :)
http://www.davidfaure.fr/kde/qt_preserve_case.diff for the record.

dfaure requested changes to this revision.Jan 11 2020, 7:54 PM
dfaure added inline comments.
src/views/dolphinview.cpp
722

This will behave wrong on /home/dfaure/Documents/dfaure
(it will remove the first dfaure instead of the second)
Substring search done by remove() is both dangerous and slow.

You could just do

const QString fileLocation = originalURL.path(QUrl::RemoveFilename);

(I'd call it directoryPath btw, in QUrl terms it's a path)

727

move it out of the loop

738

It would be simpler to keep the dot here (then both this line and the previous one become simpler).

This revision now requires changes to proceed.Jan 11 2020, 7:54 PM
ngraham updated this revision to Diff 73304.Jan 11 2020, 9:08 PM
ngraham marked 3 inline comments as done.

Address review comments

dfaure added inline comments.Jan 11 2020, 9:16 PM
src/views/dolphinview.cpp
734

path(), not toString().

Or if you really want a full URL in there then call it directoryURL. It means reparsing the whole URL though, I thought we'd just copy the URL and change the path.

739

Why not just put the "." in the format string here?

ngraham marked 2 inline comments as done.Jan 11 2020, 9:59 PM
ngraham added inline comments.
src/views/dolphinview.cpp
739

Because then originalFileName.chopped(extension.size() includes the dot from the extension because extension doesn't already include it.

ngraham updated this revision to Diff 73311.Jan 11 2020, 9:59 PM
ngraham marked an inline comment as done.
  • path() not toString()
  • Set existing URL's path rather than creating a whole new URL
ngraham marked an inline comment as done.Jan 11 2020, 9:59 PM
dfaure accepted this revision.Jan 12 2020, 9:56 AM
This revision is now accepted and ready to land.Jan 12 2020, 9:56 AM

I can do that, sure. But IMO there's a UX problem when you append "Copy of" to the beginning rather than the end. Imagine that you have a file that starts with a Z and you duplicate it. The duplicate appears near the top of the window because it begins with C. If the current view is scrollable, view scrolls up to the top to show you the newly duplicated file. This causes you to lose your place in the view, which could be annoying if you were planning to do something else with another adjacent file or perform another operation on the original file (perhaps to duplicate it a second time to perform some A/B testing on two duplicates with slightly different changes, for example). For this reason, I prefer the macOS behavior of appending "copy" to the end of the filename, not to the beginning.

Well, that depends on the current sorting mode, right? If I have sort-by-modified then the duplicate file will jump to the top of the view even if it has the "copy" suffix rather than the "Copy of" prefix, no?

Normally this shouldn't happen, since this uses CopyJob which preserves the mtime of the file.

Normally this shouldn't happen, since this uses CopyJob which preserves the mtime of the file.

Right. Sort-by-created then :-)

Well, that depends on the current sorting mode, right? If I have sort-by-modified then the duplicate file will jump to the top of the view even if it has the "copy" suffix rather than the "Copy of" prefix, no?

If you prepend "Copy of," then it jumps around when you're sorting by Name or Creation Date.

If you append "copy," then it only jumps around when you're sorting by Creation Date.

I agree, append is better.

Friendly Dolphin ping. :)

elvisangelaccio requested changes to this revision.Jan 19 2020, 4:12 PM

Well, that depends on the current sorting mode, right? If I have sort-by-modified then the duplicate file will jump to the top of the view even if it has the "copy" suffix rather than the "Copy of" prefix, no?

If you prepend "Copy of," then it jumps around when you're sorting by Name or Creation Date.

If you append "copy," then it only jumps around when you're sorting by Creation Date.

Fair enough ;)

src/views/dolphinview.cpp
729

Please add context for translators, explaining that %1 is a path.

733

QLatin1String if you want to concatenate, QStringLiteral(".%1").arg(extension) otherwise.

736–738

I'd call this variable originalExtension.

752

What's this needed for?

754–755

What's this connection needed for?

src/views/dolphinview.h
379–380

Where is the rename operation in this patch?

This revision now requires changes to proceed.Jan 19 2020, 4:12 PM
ngraham updated this revision to Diff 73973.Jan 20 2020, 5:11 PM
ngraham marked 5 inline comments as done.

Address some review comments (more to come in a bit)

ngraham added inline comments.Jan 20 2020, 5:11 PM
src/views/dolphinview.cpp
752

To make sure the new file(s) get selected after creation.

ngraham marked an inline comment as done.Jan 20 2020, 5:11 PM
ngraham updated this revision to Diff 74185.Jan 22 2020, 11:40 PM

Do rename operation in another patch later

ngraham marked an inline comment as done.Jan 22 2020, 11:41 PM

I can't figure out how to make the rename operation work right now; see the FIXME: in the code. We can do it later in another patch, or else maybe someone could help me with it here.

I don't get why we want to start a rename operation at all, tbf.

Anyway, it doesn't seems to work for me. Can you update the test plan?

ngraham edited the test plan for this revision. (Show Details)Jan 27 2020, 7:52 PM

Done.

Sorry for the delay, I moved to a new house. I'll resume all pending reviews soon.

Congratulations!!!! Take all the time you need.

elvisangelaccio requested changes to this revision.Sun, Mar 15, 5:55 PM

I'd say it's time to merge this patch since the 20.04 feature freeze is close.

Just a minor thing (see inline) and then you can push it.

src/views/dolphinview.cpp
753–758

Since this doesn't work, I'd just remove it.

(I still think that we don't need to trigger renaming in the first place).

This revision now requires changes to proceed.Sun, Mar 15, 5:55 PM
ngraham updated this revision to Diff 77673.Sun, Mar 15, 7:11 PM

Remove auto-renaming

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Mar 15, 7:14 PM
This revision was automatically updated to reflect the committed changes.