Differential D8208 Diff 73311 src/views/dolphinview.cpp

Changeset View

Standalone View

View Options

src/views/dolphinview.cpp

Show First 20 Lines • Show All 57 Lines • ▼ Show 20 Line(s)
58#include <KProtocolManager> 58​#include <KProtocolManager>
59 59​
60#include <QAbstractItemView> 60​#include <QAbstractItemView>
61#include <QApplication> 61​#include <QApplication>
62#include <QClipboard> 62​#include <QClipboard>
63#include <QDropEvent> 63​#include <QDropEvent>
64#include <QGraphicsSceneDragDropEvent> 64​#include <QGraphicsSceneDragDropEvent>
65#include <QMenu> 65​#include <QMenu>
66​#include <QMimeDatabase>
66#include <QPixmapCache> 67​#include <QPixmapCache>
67#include <QPointer> 68​#include <QPointer>
68#include <QScrollBar> 69​#include <QScrollBar>
69#include <QSize> 70​#include <QSize>
70#include <QTimer> 71​#include <QTimer>
71#include <QVBoxLayout> 72​#include <QVBoxLayout>
72 73​
73DolphinView::DolphinView(const QUrl& url, QWidget* parent) : 74​DolphinView::DolphinView(const QUrl& url, QWidget* parent) :
▲ Show 20 Lines • Show All 624 Lines • ▼ Show 20 Line(s)
698void DolphinView::pasteIntoFolder() 699​void DolphinView::pasteIntoFolder()
699{ 700​{
700 const KFileItemList items = selectedItems(); 701​ const KFileItemList items = selectedItems();
701 if ((items.count() == 1) && items.first().isDir()) { 702​ if ((items.count() == 1) && items.first().isDir()) {
702 pasteToUrl(items.first().url()); 703​ pasteToUrl(items.first().url());
703 } 704​ }
704} 705​}
705 706​
707​void DolphinView::duplicateSelectedItems()
708​{
709​ const KFileItemList itemList = selectedItems();
710​ if (itemList.isEmpty()) {
711​ return;
712​ }
713​
714​ const QMimeDatabase db;
715​
716​ // Duplicate all selected items and append "copy" to the end of the file name
717​ // but before the filename extension, if present
718​ QList<QUrl> newSelection;
719​ for (const auto &item : itemList) {
pinoUnsubmitted
Done

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

pino: using QFileInfo will break if the url is not a local file
720​ const QUrl originalURL = item.url();
721​ const QString originalFileName = item.name();
dfaureUnsubmitted
Done

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)

dfaure: This will behave wrong on /home/dfaure/Documents/dfaure (it will remove the first dfaure…
722​ QString extension = db.suffixForFileName(originalFileName);
fvogtUnsubmitted
Done

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"

fvogt: Note that this returns a null `QString` if the extension isn't known and also doesn't preserve…
723​
724​ QUrl duplicateURL = originalURL;
725​
726​ // No extension; new filename is "<oldfilename> copy"
dfaureUnsubmitted
Done

move it out of the loop

dfaure: move it out of the loop
727​ if (extension.isEmpty()) {
728​ duplicateURL.setPath(i18n("%1 copy", originalURL.path()));
elvisangelaccioUnsubmitted
Done

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

elvisangelaccio: Please add context for translators, explaining that %1 is a path.
729​ // There's an extension; new filename is "<oldfilename> copy.<extension>"
730​ } else {
731​ // Need to add a dot since QMimeDatabase::suffixForFileName() doesn't include it
732​ extension = QStringLiteral(".") + extension;
elvisangelaccioUnsubmitted
Done

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

elvisangelaccio: `QLatin1String` if you want to concatenate, `QStringLiteral(".%1").arg(extension)` otherwise.
733​ const QString directoryPath = originalURL.adjusted(QUrl::RemoveFilename).path();
dfaureUnsubmitted
Done

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.

dfaure: path(), not toString(). Or if you really want a full URL in there then call it directoryURL.
734​ const QString originalFilenameWithoutExtension = originalFileName.chopped(extension.size());
735​ // Preserve file's original filename extension in case the casing differs
736​ // from what QMimeDatabase::suffixForFileName() returned
737​ const QString suffixString = originalFileName.right(extension.size());
dfaureUnsubmitted
Done

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

dfaure: It would be simpler to keep the dot here (then both this line and the previous one become…
elvisangelaccioUnsubmitted
Done

I'd call this variable originalExtension.

elvisangelaccio: I'd call this variable `originalExtension`.
738​ duplicateURL.setPath(i18nc("<file path><filename> copy.<extension>", "%1%2 copy%3", directoryPath, originalFilenameWithoutExtension, suffixString));
dfaureUnsubmitted
Done

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

dfaure: Why not just put the "." in the format string here?
ngrahamAuthorUnsubmitted
Done

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

ngraham: Because then `originalFileName.chopped(extension.size()` includes the dot from the extension…
739​ }
740​
741​ KIO::CopyJob* job = KIO::copyAs(originalURL, duplicateURL, KIO::HideProgressInfo);
742​ KJobWidgets::setWindow(job, this);
broulikUnsubmitted
Done

You might want to look into KIO::suggestName or what it's called but I think this might not be completely suitable for this usecase here

broulik: You might want to look into `KIO::suggestName` or what it's called but I think this might not…
743​
744​ if (job) {
745​ newSelection << duplicateURL;
746​ KIO::FileUndoManager::self()->recordCopyJob(job);
747​ }
748​ }
749​
750​ forceUrlsSelection(newSelection.first(), newSelection);
751​ emitSelectionChangedSignal();
elvisangelaccioUnsubmitted
Done

What's this needed for?

elvisangelaccio: What's this needed for?
ngrahamAuthorUnsubmitted
Done

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

ngraham: To make sure the new file(s) get selected after creation.
752​
753​ connect(m_view, &DolphinItemListView::roleEditingFinished,
754​ this, &DolphinView::slotRoleEditingFinished);
elvisangelaccioUnsubmitted
Done

What's this connection needed for?

elvisangelaccio: What's this connection needed for?
755​}
756​
706void DolphinView::stopLoading() 757​void DolphinView::stopLoading()
elvisangelaccioUnsubmitted
Not Done

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).

elvisangelaccio: Since this doesn't work, I'd just remove it. (I still think that we don't need to trigger…
707{ 758​{
708 m_model->cancelDirectoryLoading(); 759​ m_model->cancelDirectoryLoading();
709} 760​}
710 761​
711void DolphinView::updatePalette() 762​void DolphinView::updatePalette()
712{ 763​{
713 QColor color = KColorScheme(isActiveWindow() ? QPalette::Active : QPalette::Inactive, KColorScheme::View).background().color(); 764​ QColor color = KColorScheme(isActiveWindow() ? QPalette::Active : QPalette::Inactive, KColorScheme::View).background().color();
714 if (!m_active) { 765​ if (!m_active) {
▲ Show 20 Lines • Show All 1144 Lines • Show Last 20 Lines